Skip to content
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

[c++11] Use std::unique_ptr for block creation. #8223

Merged
merged 1 commit into from Oct 18, 2016

Conversation

domob1812
Copy link
Contributor

CreateNewBlock returns a pointer for which the caller takes ownership. Use std::unique_ptr to make this explicit and simplify handling of these objects in getblocktemplate.

CreateNewBlock returns a pointer for which the caller takes ownership.
Use std::unique_ptr to make this explicit and simplify handling of these
objects in getblocktemplate.
@@ -164,7 +165,7 @@ CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, FormatStateMessage(state)));
}

return pblocktemplate.release();
return std::move(pblocktemplate);
Copy link
Member

Choose a reason for hiding this comment

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

Return value optimization applies here; I think using std::move is discouraged in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it does not. pblocktemplate is a member variable and not a local one, I think the code does not compile without std::move.

@sipa
Copy link
Member

sipa commented Jun 18, 2016

Oops, I forgot we just merged the refactor that turned it into a class. You're right.

@luke-jr
Copy link
Member

luke-jr commented Jun 18, 2016

Concept ACK. But it might make more sense to use a shared_ptr so TestBlockValidity can be split off to a new thread while GBT returns...

@dcousens
Copy link
Contributor

utACK 9fce062

@@ -160,7 +160,7 @@ class BlockAssembler
public:
BlockAssembler(const CChainParams& chainparams);
/** Construct a new block template with coinbase to scriptPubKeyIn */
CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn);
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the caller decide in what kind of pointer to wrap the object?
What if they want to e.g. have a shared pointer instead?

Copy link
Member

Choose a reason for hiding this comment

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

Converting from unique_ptr to shared_ptr is relatively cheap (specifically, does not require copying the object).

Copy link
Member

Choose a reason for hiding this comment

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

But does this mean we want all functions that currently return a bare pointer that will become caller ownership to returning unique_ptr?
This is probably the kind of c++11 refactoring that we want to leave for later.

@laanwj
Copy link
Member

laanwj commented Oct 18, 2016

utACK 9fce062

@laanwj laanwj merged commit 9fce062 into bitcoin:master Oct 18, 2016
laanwj added a commit that referenced this pull request Oct 18, 2016
9fce062 [c++11] Use std::unique_ptr for block creation. (Daniel Kraft)
@domob1812 domob1812 deleted the miner-uniqueptr branch October 19, 2016 15:24
codablock pushed a commit to codablock/dash that referenced this pull request Jan 12, 2018
9fce062 [c++11] Use std::unique_ptr for block creation. (Daniel Kraft)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
9fce062 [c++11] Use std::unique_ptr for block creation. (Daniel Kraft)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants