Add std::unordered_set support and a helper BuildList to dedup list build handlers that is being used for map, set and vector.
While looking bitcoin/bitcoin#29409, found a TODO noting that libmultiprocess lacked std::unordered_set support, requiring downstream that PR to implement the build/read functions locally.
I believe there could be more dedup adding a ReadList too. I could do that as a follow-up if desirable
<!--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 |
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-->
Code review ACK 00c9a8e91515bd367e28a2d5b1e235101d967aaf. This is a well-implemented change that simplifies https://github.com/bitcoin/bitcoin/pull/29409, and could help other places, since unordered_sets are used for various things in bitcoin core, and it makes sense to allow them to be easily passed over capnproto interfaces.
I believe there could be more dedup adding a ReadList too. I could do that as a follow-up if desirable
Yes this would be a nice followup, especially since type-set.h and type-unordered-set.h files are almost exact duplicates of each other and other read functions are very similar. Might be possible to write a generic ReadList that works for all containers by taking an init(size) callback and an emplace(args...) callback.
The new optional_int field on master uses a proper camelCase capnp name (optionalInt) with $Proxy.name mapping to a snake_case C++ member, which reads cleaner than the older lowercase style (setint, vbool). Should I follow that pattern here and rename unorderedsetint to an unorderedSetInt capnp field with a snake_case C++ member?
No need to touch the existing setint/vbool fields here. I believe that cleanup could be a separate PR.
Should I follow that pattern here and rename unorderedsetint to an unorderedSetInt capnp field with a snake_case C++ member?
Anything seems fine to me, would choose whatever seems cleanest to you. Feel free to include a commit renaming existing fields in this PR too if you'd like
Forced-push 9de4b885aa60b91a015f670a37bcbc64333c1d0c rebasing and fixing conflict with master and added the commit 9de4b885aa60b91a015f670a37bcbc64333c1d0c "test: use camelCase + $Proxy.name for FooStruct fields"
20 | @@ -21,10 +21,10 @@ namespace test { 21 | struct FooStruct 22 | { 23 | std::string name; 24 | - std::set<int> setint; 25 | - std::vector<bool> vbool; 26 | + std::set<int> set_int; 27 | + std::vector<bool> v_bool;
In commit "test: use camelCase + $Proxy.name for FooStruct fields" (9de4b885aa60b91a015f670a37bcbc64333c1d0c)
Not important but vector_bool would seem more consistent with other field names
I agree. If in the future I touch these code lines again I will apply this suggestion. Thanks for the feedback!
Code review ACK 9de4b885aa60b91a015f670a37bcbc64333c1d0c. Only changes since last review were renaming and adding commit user better field names in the test.