-
Notifications
You must be signed in to change notification settings - Fork 907
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
base: v2
Are you sure you want to change the base?
Conversation
A corresponding PR for capnproto-rust has now been submitted that also implements this: capnproto/capnproto-rust#436 |
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
Could we possibly take this approach and not rely on type_traits here? |
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 |
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? |
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, Then we use that type anywhere where a list of a generic parameter is expected. No need for a new I'm not sure I understand exactly how your |
The purpose of 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 So, either the |
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 parameterT
intoAnyList
. 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 aList(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 normalList(T)
interface, but actually initializes and encodes anAnyList
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 inEnableIfReader
instead of C++20requires
, 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.