Skip to content

Conversation

@fdoving
Copy link
Contributor

@fdoving fdoving commented Dec 19, 2021

Original-author: practicalswift

Use std::unique_ptr (C++11) where possible.

Rationale:

Avoid resource leaks (specifically: forgetting to delete an object created using new)
Avoid undefined behaviour (specifically: double delete:s)

Changes:
Resolving conflicts and adjustments for Ravencoin and assets.

@fdoving
Copy link
Contributor Author

fdoving commented Dec 19, 2021

TODO: Assets still need some work.

@fdoving fdoving changed the title Backport: Use std::unique_ptr (C++11) where possible (Bitcoin #11043) backport: Use std::unique_ptr (C++11) where possible (Bitcoin #11043) Dec 19, 2021
@fdoving fdoving changed the title backport: Use std::unique_ptr (C++11) where possible (Bitcoin #11043) backport: Use std::unique_ptr (C++11) where possible (bitcoin #11043) Dec 19, 2021
Copy link
Contributor

@hans-schmidt hans-schmidt left a comment

Choose a reason for hiding this comment

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

I see no justification for removing the dbMaxFileSize parameter in line 1535 of file src/init.cpp

I see no justification for deleting line #3178 of file src/validation.cpp: "assert(view.GetBestBlock() == pindexDelete->GetBlockHash());"

I have not completed reviewing the RVN-only changes in src/rpc/assets.cpp, src/rpc/rawtransactions.cpp, or src/txmempool.cpp

I question whether this type of PR is worthwhile for us to spend our limited resources on since although it may be "better" programming style, it doesn't fix any known problems, it changes consensus code, it takes significant effort, and there was even debate in bitcoin that some of these changes could potentially be risky.

practicalswift and others added 5 commits January 21, 2022 19:55
* pcoinscatcher (CCoinsViewErrorCatcher)
* pcoinsdbview (CCoinsViewDB)
* pcoinsTip (CCoinsViewCache)
* pblocktree (CBlockTreeDB)
* Remove variables shadowing pcoinsdbview
@fdoving
Copy link
Contributor Author

fdoving commented Jan 21, 2022

Re-added the assert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants