Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat List(T) with generic parameter as AnyList #1807

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from

Conversation

ErikMcClure
Copy link

This is a new iteration of the previous pull request #1608 (recreated due to v2 branch shenanigans), which generates code that transparently converts a typed List(T) with a generic parameter T into AnyList. The schema is still modified to allow describing a list with a generic parameter, which ensures that the type information can be used by the codegen, but a List(AnyPointer) is never actually encoded anywhere, which avoids accidentally creating two simultaneous possible encodings of a list of structs.

For the C++ codegen, this is accomplished via a TypedAnyList class, which pretends to be a normal List(T) interface, but actually initializes and encodes an AnyList under the hood.

However, the existence of truly generic parameters breaks some assumptions the C++ codegen was making. In particular, the ArrayInitializer codepath assumes that it knows if a type is an interface or not at codegen time, which is no longer true for a true generic parameter. This means the C++ code must be able to determine whether a type as ::Reader or ::Client at compile-time, which is done using old-fashioned C++17 SFINAE in EnableIfReader instead of C++20 requires, in case these changes need to be backported.

In addition, because it didn't seem like any code path was testing the ::Client array initializer, it simply deletes the array initializer function if the type doesn't have ::Reader - this PR can be modified to instead replace it with a ::Client version instead, if that is more appropriate. I have tried to keep the changes as minimal as possible here, so let me know if any additional cleanup is needed before this PR is accepted.

@ErikMcClure
Copy link
Author

A corresponding PR for capnproto-rust has now been submitted that also implements this: capnproto/capnproto-rust#436

@kentonv
Copy link
Member

kentonv commented Nov 1, 2023

Sorry for the long delay in replying, I just moved, was out-of-office for a few weeks, and have been struggling to keep up with email.

Thanks for working on this.

It looks like you've used std's type_traits a bunch here, but the rest of KJ and Cap'n Proto has always avoided type_traits and more generally avoided including std headers within its own headers, with a (perhaps unrealistic) goal that KJ could eventually be fully independent of std.

In general I've found it's pretty easy to express most things using raw C++ templates with a few helpers found in kj/common.h. For example, to enable a template only for types that have a Reader I'd normally do:

template <typename T, typename = T::Reader>

Could we possibly take this approach and not rely on type_traits here?

@ErikMcClure
Copy link
Author

Using your suggestion, I was able to use a much more direct SFINAE route which seems to work on both MSVC and Clang (and GCC in a toy example: https://godbolt.org/z/TncbaGoG4):

  template<typename U = Foo, typename = typename U::Reader> inline void setListResult(::kj::ArrayPtr<const typename U::Reader> value);

I've updated the PR to use this new method, which allowed me to revert all my changes to generated-header-support.h.

@ErikMcClure
Copy link
Author

Were there any additional changes that needed to happen for this to be merged? Would it be helpful to backport this to the 1.x branch?

@kentonv
Copy link
Member

kentonv commented Jan 12, 2024

Sorry for slow response again. It's hard to find time for complicated code reviews that aren't in the critical path for my day job. :/ (And it always takes me at least 15 minutes just to remember what's going on in this one...)

So what I had expected to see here is something like:

template <typename T>
struct ListMaybeAny_ {
  using Type = List<T>;
};
template <>
struct ListMaybeAny_<AnyPointer> {
  using Type = AnyList;
};

template <typename T>
using ListMaybeAny = ListMaybeAny_<T>::Type;

That is, ListMaybeAny<T> is an alias for List<T> except in the case of ListMaybeAny<AnyPointer> which instead becomes AnyList.

Then we use that type anywhere where a list of a generic parameter is expected. No need for a new PointerHelpers specialization, I think, since this type just aliases other types that already have helpers.

I'm not sure I understand exactly how your TypedAnyList solution works, since it seems like TypedAnyList<AnyPointer>::Builder will still alias to List<AnyPointer>::Builder. This leads to problems because List<AnyPointer>::Builder is inappropriate for manipulating lists of structs.

@ErikMcClure
Copy link
Author

The purpose of TypedAnyList PointerHelpers specialization was actually to deal with the result of trying to use ListMaybeAny<T>, because it generates this initialization code in the test:

template <typename Foo, typename Bar>
inline typename  ::capnp::ListMaybeAny<Foo>::Builder TestGenerics<Foo, Bar>::Builder::initListFoo(unsigned int size) {
  return ::capnp::_::PointerHelpers< ::capnp::AnyList>::init(_builder.getPointerField(
      ::capnp::bounded<3>() * ::capnp::POINTERS), size);
}

This fails to compile when Foo is set to AnyPointer because the init function for AnyList is completely different, since it doesn't have the type information. TypedAnyList was created to forward the type information from the init function to the AnyList init function.

So, either the init function needs to be modified somehow, or I am inserting ListMaybeAny into the wrong location. If this is the wrong place to insert ListMaybeAny, I admit that I'm a bit lost as to where else I would put it. Did you have something else in mind for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants