Add ReadList helper

[ViniciusCestarii]

Follow up of #277 (review). This adds ReadList helper and uses it to dedup the CustomReadField implementations for std::map, std::set, std::unordered_set, and std::vector, mirroring the existing BuildList helper on the build side.

I took the liberty of adding the commit f2a734a "type: reserve first when reading std::unordered_set" so it reserves the correct capacity first and then emplaces the values.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	ryanofsky
Stale ACK	xyzconstant

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #288 (Create support branch for CI scripts, documentation, and examples by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ryanofsky]

Concept ACK f2a734a. Thanks for the followup. Plan to review more after #277 is merged

[ViniciusCestarii]

Forced push 245f792 to just rebase with master

[ViniciusCestarii]

Forced push 5d23f83 applying @ryanofsky suggestion. It reads cleaner and dedup more code.

[ViniciusCestarii]

Forced push 043b8a8 applying @xyzconstant suggestion

[xyzconstant]

tACK 043b8a8

Thanks for the updates @ViniciusCestarii.

This is a simple refactor that moves common code to ReadList, consistent with existing BuildList helper.

The changes are clean and can be merged as-is.

[ryanofsky]

In commit "proxy: add ReadList helper and dedup map/set/vector read handlers" (1bfc594)

There seems to be a regression here. It would be better for this to return decltype(auto) and use return read_dest.update(...) below so these CustomReadField functions are compatible with ReadDestTemp. ReadDestTemp is not documented or covered very well from the test framework right now but the idea behind it is to support returning types like UniValue::type_error that can't be modified after they are constructed.

Fortunately the current unit tests do include one cases where ReadDestTemp is used (even though it isn't really needed and ReadDestUpdate would be a better fit), so it can be extended to test ReadList return value. A full fix and test is below that will result in a compile error if ReadList returns void. Feel free to only add the return value fix here, since the test could be a followup and I started working on a separate change try to test ReadDestTemp more fully. Also feel free to add the test to this PR. I would just suggest if adding it, to add it as a new initial commit before the other changes so the refactoring is separate.

diff

diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h
index 66310d9..f1127da 100644
--- a/include/mp/proxy-types.h
+++ b/include/mp/proxy-types.h
@@ -292,9 +292,9 @@ void BuildList(TypeList<LocalType>, InvokeContext& invoke_context, Output&& outp
 }
 
 template <typename LocalType, typename Input, typename ReadDest, typename InitFn, typename EmplaceFn>
-void ReadList(TypeList<LocalType>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest, InitFn&& init, EmplaceFn&& emplace)
+decltype(auto) ReadList(TypeList<LocalType>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest, InitFn&& init, EmplaceFn&& emplace)
 {
-    read_dest.update([&](auto& value) {
+    return read_dest.update([&](auto& value) {
         auto data = input.get();
         init(value, data.size());
         for (auto item : data) {
diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h
index b96eabf..1bc6c52 100644
--- a/test/mp/test/foo-types.h
+++ b/test/mp/test/foo-types.h
@@ -46,6 +46,7 @@ void CustomBuildField(TypeList<FooCustom>, Priority<1>, InvokeContext& invoke_co
 {
     BuildField(TypeList<std::string>(), invoke_context, output, value.v1);
     output.setV2(value.v2);
+    BuildField(TypeList<std::vector<int>>(), invoke_context, output, value.v3);
 }
 
 template <typename Input, typename ReadDest>
@@ -55,6 +56,7 @@ decltype(auto) CustomReadField(TypeList<FooCustom>, Priority<1>, InvokeContext&
     return read_dest.update([&](FooCustom& value) {
         value.v1 = ReadField(TypeList<std::string>(), invoke_context, mp::Make<mp::ValueField>(custom.getV1()), ReadDestTemp<std::string>());
         value.v2 = custom.getV2();
+        value.v3 = ReadField(TypeList<std::vector<int>>(), invoke_context, mp::Make<mp::ValueField>(custom.getV3()), ReadDestTemp<std::vector<int>>());
     });
 }
 
diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp
index 3173645..89d6f90 100644
--- a/test/mp/test/foo.capnp
+++ b/test/mp/test/foo.capnp
@@ -64,6 +64,7 @@ struct FooStruct $Proxy.wrap("mp::test::FooStruct") {
 struct FooCustom $Proxy.wrap("mp::test::FooCustom") {
     v1 @0 :Text;
     v2 @1 :Int32;
+    v3 @2 :List(Int32);
 }
 
 struct FooEmpty $Proxy.wrap("mp::test::FooEmpty") {
diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h
index 5b66b85..54fd043 100644
--- a/test/mp/test/foo.h
+++ b/test/mp/test/foo.h
@@ -33,6 +33,7 @@ struct FooCustom
 {
     std::string v1;
     int v2;
+    std::vector<int> v3;
 };
 
 struct FooEmpty
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
index eb4b5ec..86fd1bf 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -214,9 +214,11 @@ KJ_TEST("Call FooInterface methods")
     FooCustom custom_in;
     custom_in.v1 = "v1";
     custom_in.v2 = 5;
+    custom_in.v3 = {10, 20, 30};
     FooCustom custom_out = foo->passCustom(custom_in);
     KJ_EXPECT(custom_in.v1 == custom_out.v1);
     KJ_EXPECT(custom_in.v2 == custom_out.v2);
+    KJ_EXPECT(custom_in.v3 == custom_out.v3);
 
     foo->passEmpty(FooEmpty{});
 

[ViniciusCestarii]

Nice thanks for the explanation, I only aplied the return decltype(auto) bit on 4d0f8db, so all the tests could be in its own PR

[ryanofsky]

In commit "proxy: add ReadList helper and dedup map/set/vector read handlers" (1bfc594)

I think there's a nicer way to handle the vector<bool> case here. Just replace auto& with decltype(auto) and returning value.back() will no longer be an error. The return value will not actually be used because the ReadField call which is reading the bools in ReadList is going call the integer readfield overload, which calls read_dest.construct and passes the bool to this emplace lambda without using the return value.

Suggested change

--- a/include/mp/type-vector.h
+++ b/include/mp/type-vector.h
@@ -37,34 +37,11 @@ decltype(auto) CustomReadField(TypeList<std::vector<LocalType>>,
             value.clear();
             value.reserve(size);
         },
-        [&](auto& value, auto&&... args) -> auto& {
+        [&](auto& value, auto&&... args) -> decltype(auto) {
             value.emplace_back(std::forward<decltype(args)>(args)...);
             return value.back();
         });
 }
-
-//! Overload CustomReadField for std::vector<bool>, which needs its own handler
-//! because its back() returns a proxy by value (std::vector<bool>::reference)
-//! rather than a reference, so it can't use the generic vector in-place emplace path
-//! that returns auto&. Not returning a reference is fine here: bool is read via
-//! construct(), not update(), so the return value is never used.
-template <typename Input, typename ReadDest>
-decltype(auto) CustomReadField(TypeList<std::vector<bool>>,
-                               Priority<1>,
-                               InvokeContext& invoke_context,
-                               Input&& input,
-                               ReadDest&& read_dest)
-{
-    return ReadList(
-        TypeList<bool>(), invoke_context, input, read_dest,
-        [&](auto& value, size_t size) {
-            value.clear();
-            value.reserve(size);
-        },
-        [&](auto& value, auto&& item) {
-            value.push_back(item);
-        });
-}
 } // namespace mp
 
 #endif // MP_PROXY_TYPE_VECTOR_H

[ViniciusCestarii]

I agree it is nicer. Done on 4d0f8db

[ryanofsky]

In commit "type: reserve first when reading std::unordered_set" (043b8a8)

Note: new code is dropping const LocalType here which I could look like a problem, because std::set members are const and can't be updated. So this might seem to break cases like std::set<std::shared_ptr<T>> because shared_ptr readfield uses read_dest.update and would be unable to update the new emplaced set element. But this works because the is_const_v check ignores LocalType entirely and just checks type of the return value of the emplace function. And that is still const below (*value.emplace(std::forward<decltype(args)>(args)...).first is const).

[ViniciusCestarii]

Thanks for reviewing. Forced push 6450345 applying @ryanofsky suggestions