Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Dec 7, 2020

The constructor is confusing and dangerous (as explained in the TODO), fix that by removing it.

@laanwj
Copy link
Member

laanwj commented Dec 7, 2020

I kind of wish CreateTransaction would simply return an optional<transaction, …> instead of the in/out parameters (of which one a reference to a transaction reference), that would make it easier to review.

@maflcko maflcko force-pushed the 2012-txConstructor branch from c86ecca to fac39c1 Compare December 7, 2020 14:03
@practicalswift
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Dec 10, 2020

Thank, looks good to me now.
Code review ACK fac39c1

Copy link
Contributor

@promag promag 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 fac39c1.

Copy link
Contributor

@theStack theStack 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 fac39c1

@fanquake fanquake merged commit ade38b6 into bitcoin:master Dec 13, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 13, 2020
@maflcko maflcko deleted the 2012-txConstructor branch December 13, 2020 06:12
ryanofsky added a commit to ryanofsky/libmultiprocess that referenced this pull request Dec 18, 2020
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.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants