Add std::unordered_set support and a helper BuildList to dedup list build handlers#277
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
ViniciusCestarii
force-pushed
the
add-unordered-set-support
branch
from
e1914bb to
5b9db34
Compare
ViniciusCestarii
force-pushed
the
add-unordered-set-support
branch
from
5b9db34 to
00c9a8e
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 00c9a8e. This is a well-implemented change that simplifies bitcoin/bitcoin#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. |
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 |
ViniciusCestarii
force-pushed
the
add-unordered-set-support
branch
from
00c9a8e to
81e508f
Compare
ViniciusCestarii
force-pushed
the
add-unordered-set-support
branch
from
81e508f to
9de4b88
Compare
| std::set<int> setint; | ||
| std::vector<bool> vbool; | ||
| std::set<int> set_int; | ||
| std::vector<bool> v_bool; |
There was a problem hiding this comment.
In commit "test: use camelCase + $Proxy.name for FooStruct fields" (9de4b88)
Not important but vector_bool would seem more consistent with other field names
There was a problem hiding this comment.
I agree. If in the future I touch these code lines again I will apply this suggestion. Thanks for the feedback!
8412fcdc65 Merge bitcoin-core/libmultiprocess#295: Mark Waiter m_cv as guarded by m_mutex 1593ee2d18 Merge bitcoin-core/libmultiprocess#294: test: Add passDouble smoke test 9885d7dd33 Merge bitcoin-core/libmultiprocess#286: proxy-client: fix TSan data race in clientDestroy fa35501c4f Mark Waiter m_cv as guarded by m_mutex faaedb11f8 test: Add passDouble smoke test 733c64318d Merge bitcoin-core/libmultiprocess#292: type-number: fix clang-tidy modernize-use-nullptr 9cc3479ab3 Merge bitcoin-core/libmultiprocess#291: cmake: Add `mp_headers` custom target 201abd9e3a Merge bitcoin-core/libmultiprocess#289: cmake: make target_capnp_sources use CURRENT dirs 99820c8aec Merge bitcoin-core/libmultiprocess#279: doc: Add comments to FIELD_* constants in proxy.h 73b985540c Merge bitcoin-core/libmultiprocess#278: doc: Fix and expand design.md e7e91b2e23 Merge bitcoin-core/libmultiprocess#277: Add std::unordered_set support and a helper BuildList to dedup list build handlers 91a951f59a tidy fix: modernize-use-nullptr 16362f42d0 cmake: Add `mp_headers` custom target 615a94fe3a cmake: document ONLY_CAPNP option in target_capnp_sources 90982f75c6 mpgen: iwyu changes required by previous commit 25bb3e67f3 proxy-client: fix TSan data race in clientDestroy 620f297f31 cmake: make target_capnp_sources use CURRENT dirs 9de4b885aa test: use camelCase + $Proxy.name for FooStruct fields 011b91793d type: add std::unordered_set support 20d19b9644 proxy: add BuildList helper and dedup map/set/vector build handlers e863c6cdf6 doc: Add comments to FIELD_* constants in proxy.h 18db0ab957 doc: Fix and expand design.md 61de697536 Merge bitcoin-core/libmultiprocess#273: proxy-client: tolerate exceptions from remote destroy during cleanup 9cec9d6ca5 Merge bitcoin-core/libmultiprocess#243: mpgen: support primitive std::optional struct fields 4aaff11374 Merge bitcoin-core/libmultiprocess#238: cmake, ci: updates for recent nixpkgs 2ac55a56b5 Merge bitcoin-core/libmultiprocess#218: Better error and log messages 6de92e1c73 proxy-client: tolerate exceptions from remote destroy during cleanup 90be8354d4 test: regression for ~ProxyClient destroy after peer disconnect 3c69d125a1 Merge bitcoin-core/libmultiprocess#260: event loop: tolerate unexpected exceptions in `post()` callbacks b8a48c65e6 event loop: tolerate unexpected exceptions in `post()` callbacks f787863d2c Merge bitcoin-core/libmultiprocess#270: doc: Bump version 10 > 11 a22f602910 doc: Bump version 10 > 11 4eae445d6d debug: Add TypeName() function and log statements for Proxy objects being created and destroyed f326c5b1b7 logging: Add better logging on IPC server-side failures 6dbfa56a04 mpgen: support primitive std::optional struct fields 8d1277deb5 mpgen refactor: add AccessorType function db716bbcba mpgen refactor: Move field handling code to FieldList class db7acb3ce2 ci: Fix shell.nix compatibility with CMake 4.0 91a7759a9a cmake: Fix IWYU in nix by adding CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 8412fcdc659e1379f9b4dea896c26bc1c5f3afa8
3 participants
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