[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
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]

re: #285 (comment)

all the tests could be in its own PR

#338 follows up with this test

[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

[ryanofsky]

Code review ACK 6450345. Looks great! It's really nice to deduplicate this code and drop vector<bool> specialization.

I have a bunch of commits building on top of this to test the ReadDestTemp use-case that was broken by the earlier version of this PR, also to provide much better documentation for the ReadField/ReadDest code in general, and also to add better support and test coverage for non-default constructible and non-movable types, so full ReadField capabilities are really exercised within libmultiprocess, and not just used downstream in bitcoin/bitcoin#10102 and easily broken by changes like this.

[ryanofsky]

re: #285 (review)

I have a bunch of commits building on top of this to test the ReadDestTemp use-case that was broken by the earlier version of this PR, also to provide much better documentation for the ReadField/ReadDest code in general, and also to add better support and test coverage for non-default constructible and non-movable types, so full ReadField capabilities are really exercised within libmultiprocess, and not just used downstream in bitcoin/bitcoin#10102 and easily broken by changes like this.

These are cleaned up now and in #337 in #338

[ryanofsky]

re: #285 (comment)

all the tests could be in its own PR

#338 follows up with this test