Follow up of #277#pullrequestreview-4412854098. 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 f2a734a9c1515435743bba0fb9ef889687fc391c "type: reserve first when reading std::unordered_set" so it reserves the correct capacity first and then emplaces the values.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
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 <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
65 | - ReadDestEmplace( 66 | - TypeList<std::pair<const KeyLocalType, ValueLocalType>>(), [&](auto&&... args) -> auto& { 67 | - return *EmplacePiecewiseSafe(value, std::forward<decltype(args)>(args)...).first; 68 | - })); 69 | - } 70 | + ReadList(
In commit "proxy: add ReadList helper and dedup map/set/vector read handlers" (2bc6cb761fbccbea5600ec775f23274041aab599)
Not sure if possible but it might be good to pass read_dest instead of value to the ReadList function so callers don't need to call read_dest.update and repeat as much code. This would make ReadList and BuildList more similar in how they are called. Looks like this would require adding auto& value parameters to init and emplace functions, which could also be good because it will declare value closer to where it is used.
True, I will take a look on this approach
Forced push 245f7927742fad0ed3e5f2fe3b5fc246f58e2e78 to just rebase with master
Forced push 5d23f83e99de31cf932a58e51b29dd06240c70ac applying @ryanofsky suggestion. It reads cleaner and dedup more code.
Does this work here?
diff --git a/include/mp/type-vector.h b/include/mp/type-vector.h
index 25e74a6..cd269cb 100644
--- a/include/mp/type-vector.h
+++ b/include/mp/type-vector.h
@@ -50,14 +50,16 @@ decltype(auto) CustomReadField(TypeList<std::vector<bool>>,
Input&& input,
ReadDest&& read_dest)
{
- return read_dest.update([&](auto& value) {
- auto data = input.get();
- value.clear();
- value.reserve(data.size());
- for (auto item : data) {
- value.push_back(ReadField(TypeList<bool>(), invoke_context, Make<ValueField>(item), ReadDestTemp<bool>()));
- }
- });
+ 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);
+ return value.back();
+ });
}
} // namespace mp
oh I missed this case, thanks for pointing!
I tested your approach and it does compile and passes the tests but I think it is better to not add return value.back(); because bool is read via construct(), so the return value is always unused.
Also I think it is a good opportunity to add a comment to explain why there's this exception for std::vector<bool>.
What do you think?
diff --git a/include/mp/type-vector.h b/include/mp/type-vector.h
index 25e74a6..49ca3c2 100644
--- a/include/mp/type-vector.h
+++ b/include/mp/type-vector.h
@@ -43,6 +43,11 @@ decltype(auto) CustomReadField(TypeList<std::vector<LocalType>>,
});
}
+//! 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>,
@@ -50,14 +55,15 @@ decltype(auto) CustomReadField(TypeList<std::vector<bool>>,
Input&& input,
ReadDest&& read_dest)
{
- return read_dest.update([&](auto& value) {
- auto data = input.get();
- value.clear();
- value.reserve(data.size());
- for (auto item : data) {
- value.push_back(ReadField(TypeList<bool>(), invoke_context, Make<ValueField>(item), ReadDestTemp<bool>()));
- }
- });
+ 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
Great! This is definitely better, and the comment is on point.
Done on 1bfc59400b3d3a89012ac9591c38df121950185c.
Forced push 043b8a8fd375a77802d081e36c1ae87f324bf2cc applying @xyzconstant suggestion
tACK 043b8a8fd375a77802d081e36c1ae87f324bf2cc
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.
290 | @@ -291,6 +291,21 @@ void BuildList(TypeList<LocalType>, InvokeContext& invoke_context, Output&& outp 291 | } 292 | } 293 | 294 | +template <typename LocalType, typename Input, typename ReadDest, typename InitFn, typename EmplaceFn> 295 | +void ReadList(TypeList<LocalType>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest, InitFn&& init, EmplaceFn&& emplace)
In commit "proxy: add ReadList helper and dedup map/set/vector read handlers" (1bfc59400b3d3a89012ac9591c38df121950185c)
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.
<details><summary>diff</summary> <p>
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](/bitcoin-core-multiprocess/contributor/0/) :Text;
v2 [@1](/bitcoin-core-multiprocess/contributor/1/) :Int32;
+ v3 [@2](/bitcoin-core-multiprocess/contributor/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{});
</p> </details>
Nice thanks for the explanation, I only aplied the return decltype(auto) bit on 4d0f8db5f993de9d4fe2337bb31a9cc7a0f4a326, so all the tests could be in its own PR
47 | + TypeList<LocalType>(), invoke_context, input, read_dest, 48 | + [&](auto& value, size_t size) { 49 | + value.clear(); 50 | + value.reserve(size); 51 | + }, 52 | + [&](auto& value, auto&&... args) -> auto& {
In commit "proxy: add ReadList helper and dedup map/set/vector read handlers" (1bfc59400b3d3a89012ac9591c38df121950185c)
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
I agree it is nicer. Done on 4d0f8db5f993de9d4fe2337bb31a9cc7a0f4a326
25 | @@ -26,16 +26,12 @@ decltype(auto) CustomReadField(TypeList<std::set<LocalType>>, 26 | Input&& input, 27 | ReadDest&& read_dest) 28 | { 29 | - return read_dest.update([&](auto& value) { 30 | - auto data = input.get(); 31 | - value.clear(); 32 | - for (auto item : data) { 33 | - ReadField(TypeList<LocalType>(), invoke_context, Make<ValueField>(item), 34 | - ReadDestEmplace(TypeList<const LocalType>(), [&](auto&&... args) -> auto& {
In commit "type: reserve first when reading std::unordered_set" (043b8a8fd375a77802d081e36c1ae87f324bf2cc)
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).
Thanks for reviewing. Forced push 6450345c985cecd623951af41f0de9414e523e14 applying @ryanofsky suggestions
Code review ACK 6450345c985cecd623951af41f0de9414e523e14. 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 https://github.com/bitcoin/bitcoin/pull/10102 and easily broken by changes like this.