-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Remove unused and confusing CTransaction constructor #20588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I kind of wish |
c86ecca
to
fac39c1
Compare
Concept ACK |
Thank, looks good to me now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK fac39c1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK fac39c1
CTransaction default constructor was removed in faac31521bb7ecbf999541cf918d3750ff589de4 bitcoin/bitcoin#20588 which exposed two places where libmultiprocess was inadvertently assuming default constructors were defined. In both places, it would never actually call the default constructor at runtime, just rely on it being present during compile time code generation. In the first case, compiler would fail while trying to figure out the type of `std::invoke_result_t<EmplaceFn>` where EmplaceFn called `make_shared<CTransaction>()`. This was fixed by just the moving expression into an unexpanded template method. In the second case code was calling Optional<CTransaction>::emplace() to default-initialize CTransaction& output-arguments. It was fixed by moving into a constexpr-if that would be known to be false for output arguments at compile time.
The constructor is confusing and dangerous (as explained in the TODO), fix that by removing it.