proxy-types: Remove requirement for return types to be default-constructible - #337
Open
ryanofsky wants to merge 4 commits into
Open
proxy-types: Remove requirement for return types to be default-constructible#337ryanofsky wants to merge 4 commits into
ryanofsky wants to merge 4 commits into
Conversation
…turn types Previously the generated client declared a default-constructed result variable and passed a reference to it into clientInvoke, requiring the return type to be default-constructible. Restructure the client return path so the value is constructed directly in place: - clientInvoke takes the return type and result Accessor as explicit template arguments (defaulting to void). It deserializes the result with ReadField into a ReadDestTemp, so the value is built from constructor arguments without a default constructor. - Propagate the value as a prvalue: IterateFieldsHelper::handleChain now returns decltype(auto), and the movable path uses C++17 guaranteed copy elision to construct the result in AlignedStorage. - For non-movable return types (no move or copy constructor), copy the capnp response to a flat word buffer on the event-loop thread and deserialize it on the client thread, returning a prvalue via guaranteed copy elision so no move constructor is ever needed. Add AlignedStorage<T>, a typed wrapper around an aligned byte buffer with a ptr() accessor, used immediately by clientInvoke and reused later by TryFinally. Extend FunctionTraits to all four PMF cv-qualifier combinations so clientInvoke can derive the capnp Results type from the request method pointer. The code generator emits clientInvoke<Result, Accessor>(...) for non-void methods and plain clientInvoke(...) for void methods. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restructure result handling on the server side to eliminate move-constructor requirements for IPC return types. (Client side move-constructor requirements were removed in the previous commit.) - Add 3-arg TryFinally(fn, after, consume) overload in util.h that stores fn()'s return value via placement new from prvalue (C++17 guaranteed copy elision) and passes it by reference to consume(), then destroys it. This avoids ever needing to move the result. - Merge ServerRet into ServerCall by making ServerCall a template parameterized on the result Accessor (void for void methods). The result is now serialized inside the TryFinally after() callback, forwarding it with the invoked method's value category so move-only results (e.g. vector<unique_ptr<Bar>>) are moved rather than copied. - Update code generator to emit ServerCall<void>() or ServerCall<Accessor>() instead of Make<ServerRet, Accessor>(ServerCall()). Behavior-preserving: results are serialized exactly as before. These changes are combined because they are tightly coupled: redesigning TryFinally to return void forces ServerCall::invoke() to also return void, which breaks ServerRet's existing `auto&& result = Parent::invoke(...)` binding. The only clean intermediate would have ServerRet duplicate the same logic that ServerCall<Accessor> ends up with, so combining produces a simpler result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add Pinned<T>, a type with no default constructor and no copy or move operations, to exercise the non-movable IPC return path end-to-end. Its CustomReadField uses read_dest.construct() with a ReadDestTemp<T> argument, which is the only way to deserialize a type that can neither be default-constructed (for update()) nor moved (for a std::optional staging variable). Add two FooInterface methods: - returnPinned returns Pinned<std::vector<int>> by value, verifying the client can retrieve a non-movable return value. - throwPinned throws Pinned<std::vector<int>> via $Proxy.exception, verifying the exception path constructs a non-movable value as a prvalue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the terse ReadDestEmplace comment with structured documentation of the three ReadField destination types (ReadDestEmplace, ReadDestUpdate, and the ReadDestTemp() helper), covering: - The contract for CustomReadField implementors: return decltype(auto) and forward the construct()/update() return value, which is easy to miss because most callers ignore it but is load-bearing when ReadDestTemp() is used. - The contract for emplace callbacks and the three return-type cases (container emplace, ReadDestTemp() prvalue, and the vector<bool> reference-like proxy). - When ReadDestTemp() is merely convenient versus strictly necessary, with the Pinned<T> return-value and nested-CustomReadField cases as examples. - Return-value notes on each construct()/update() method and an inline note flagging the placement-new exception-safety gap in ReadDestUpdate::construct. Comment-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve
clientInvoketo avoid requirement that return types are default-constructible. This lets bitcoin/bitcoin#10102 return types likePartiallySignedTransaction,WalletAddress,util::Resultthat don't have default constructors, without needing to wrap them or give them default constructors.Also remove requirement that return types are copyable or movable, and add tests for this, and improve documentation for
ReadDestclasses to describe how they are supposed to be passed toReadFieldcallers and used insideCustomReadFieldimplementations to deal with different types including non-movable, non-default-constructible ones.