Skip to content

Add std::unordered_set support and a helper BuildList to dedup list build handlers#277

Open
ViniciusCestarii wants to merge 3 commits into
bitcoin-core:masterfrom
ViniciusCestarii:add-unordered-set-support
Open

Add std::unordered_set support and a helper BuildList to dedup list build handlers#277
ViniciusCestarii wants to merge 3 commits into
bitcoin-core:masterfrom
ViniciusCestarii:add-unordered-set-support

Conversation

@ViniciusCestarii
Copy link
Copy Markdown
Contributor

@ViniciusCestarii ViniciusCestarii commented May 14, 2026

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

@DrahtBot
Copy link
Copy Markdown

DrahtBot commented May 14, 2026

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
Stale ACK ryanofsky

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)
  • #276 (build: prepare for subtree split by Sjors)

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.

Copy link
Copy Markdown
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ViniciusCestarii
Copy link
Copy Markdown
Contributor Author

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.

@ryanofsky
Copy link
Copy Markdown
Collaborator

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

@ViniciusCestarii ViniciusCestarii force-pushed the add-unordered-set-support branch from 00c9a8e to 81e508f Compare June 3, 2026 17:04
@ViniciusCestarii ViniciusCestarii force-pushed the add-unordered-set-support branch from 81e508f to 9de4b88 Compare June 3, 2026 17:06
@ViniciusCestarii
Copy link
Copy Markdown
Contributor Author

Forced-push 9de4b88 rebasing and fixing conflict with master and added the commit 9de4b88 "test: use camelCase + $Proxy.name for FooStruct fields"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants