mpgen: support primitive std::optional struct fields#243
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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2026-03-09 18:50:31 |
|
It would be useful to demonstrate that other implementations can straightforwardly handle this, e.g. with a functional tests in a Bitcoin Core branch (or PR). |
My next rebase of bitcoin/bitcoin#10102 will use this for the In the meantime I updated the PR description with a more specific example of how this can be used and I think the unit test should provide a good test of functionality. Updated 9f99c7d -> 4de4429 ( Updated 4de4429 -> 26fdbad ( Rebased 26fdbad -> 6dbfa56 ( |
This is a move-only change that should be easy to review with --color-moved. No behavior is changing.
This doesn't change generated code at all, just noves functionality out of Generate method into a helper method so it can be reused in the next commit.
Currently optional primitive fields like `std::optional<int>` are not well supported as struct members. Non-primitive optional fields like `std::optional<std::string>` and optional struct fields are well-supported because Cap'n Proto allows non-primitive fields to be unset, but primitive fields are always considered set so there is natural way to represent null values. Libmultiprocess does already support primitive optional method parameters and result values, by allowing the .capnp files to declare extra boolean parameters prefixed with "has" and treating the extra boolean parameters as indicators of whether options are set or unset. This commit just this functionality to work for struct members as well. For example a C++ `std::optional<int> param` parameter can be represented by 'param :Int32, hasParam :Bool` parameters in a .capnp file and libmultiprocess will use both Cap'n Proto fields together to represent the C++ value. Now C++ struct fields can be represented the same way (see unit changes test for an example).
|
Tested ACK 6dbfa56 Built and tests pass on macOS. |
|
Concept ACK |
There was a problem hiding this comment.
I think this is a useful extension of the existing hasX convention to wrapped struct fields, and I believe it would be a net positive for downstream libmultiprocess users.
My understanding is that this teaches mpgen to represent primitive std::optional struct members the same way optional method params/results are already represented today: a value field plus a hasValue boolean. I think that is a sensible direction because it reuses an existing convention instead of introducing a separate encoding rule for struct fields.
| continue; | ||
| } | ||
| auto field_name = field.getProto().getName(); | ||
| for (const auto& field : fields.fields) { |
There was a problem hiding this comment.
In commit "mpgen: support primitive std::optional struct fields" (6dbfa56):
I think the hasX field here is intended as internal serialization plumbing for the logical optional field, so I would have expected it to stay hidden from the generated ProxyStruct surface rather than also getting its own public accessor alias.
I believe a minimal fix would be to skip alias generation for field.skip entries in the ProxyStruct loop, similar to how they are already skipped when building the Accessors tuple below. Something along these lines:
for (const auto& field : fields.fields) {
if (field.skip) continue;
auto field_name = field.param.getProto().getName();
add_accessor(field_name);
dec << " using " << Cap(field_name) << "Accessor =
"
<< AccessorType(base_name, field) << ";\n";
There was a problem hiding this comment.
re: #243 (comment)
I think the
hasXfield here is intended as internal serialization plumbing for the logical optional field, so I would have expected it to stay hidden from the generatedProxyStructsurface rather than also getting its own public accessor alias.
Yes originally, the PR took an approach like you suggested, but it was changed in a later push https://github.com/ryanofsky/libmultiprocess/compare/pr/optint.2..pr/optint.3 because it broke bitcoin/bitcoin#10102. Specifically, I think it broke this code trying to use the accessor of a skipped field:
https://github.com/ryanofsky/bitcoin/blob/pr/ipc.232/src/ipc/capnp/node.capnp#L172
https://github.com/ryanofsky/bitcoin/blob/pr/ipc.232/src/ipc/capnp/node-types.h#L87
Currently, the Field::skip struct member is a little overloaded. It is true for has & want fields but it is also true for fields where $Proxy.skip was explicitly used. And it is useful to have accessors for $Proxy.skip fields to write custom serialization code, even it if it is probably not useful to have accessors for has & want fields.
Since outputting accessors does not cost much, though, I think it should not be a problem to have a few extra one even if they are less likely to be used.
61de697 Merge bitcoin-core/libmultiprocess#273: proxy-client: tolerate exceptions from remote destroy during cleanup 9cec9d6 Merge bitcoin-core/libmultiprocess#243: mpgen: support primitive std::optional struct fields 4aaff11 Merge bitcoin-core/libmultiprocess#238: cmake, ci: updates for recent nixpkgs 2ac55a5 Merge bitcoin-core/libmultiprocess#218: Better error and log messages 6de92e1 proxy-client: tolerate exceptions from remote destroy during cleanup 90be835 test: regression for ~ProxyClient destroy after peer disconnect 3c69d12 Merge bitcoin-core/libmultiprocess#260: event loop: tolerate unexpected exceptions in `post()` callbacks b8a48c6 event loop: tolerate unexpected exceptions in `post()` callbacks f787863 Merge bitcoin-core/libmultiprocess#270: doc: Bump version 10 > 11 a22f602 doc: Bump version 10 > 11 4eae445 debug: Add TypeName() function and log statements for Proxy objects being created and destroyed f326c5b logging: Add better logging on IPC server-side failures 6dbfa56 mpgen: support primitive std::optional struct fields 8d1277d mpgen refactor: add AccessorType function db716bb mpgen refactor: Move field handling code to FieldList class db7acb3 ci: Fix shell.nix compatibility with CMake 4.0 91a7759 cmake: Fix IWYU in nix by adding CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 61de6975362a7070276da47cc2aa2c2b8909ed49
5 participants
Currently C++ structs with primitive
std::optionalmembers (ints, bools, floats) cannot easily by mapped to Cap'n Proto structs because Cap'n Proto does not provide a way to leave primitive fields unset, so there isn't a natural way to representstd::nulloptvalues. This PR makes it possible to map C++ structs with fields like:std::optional<int> foo;to Cap'n Proto structs by using extra
Boolfields prefixed with "has" for primitive optional members:Boolean "has" fields were already supported by the code generator and used to pass primitive
std::optionalparameters and return values, so this PR just extends it work with all struct fields, not just fields in params and result structs.Note: Motivation for this change is dealing with the
CreatedTransactionResult::change_posfield introduced to the wallet interface in bitcoin-core/gui#807. This also could have been useful in bitcoin/bitcoin#33965 (comment)