Use std::unique_ptr (C++11) where possible.
Rationale:
delete an object created using new)delete:s)Note to reviewers: Please let me know if I’ve missed any obvious std::unique_ptr candidates. Hopefully this PR should cover all the trivial cases.
998- delete shortStats;
999- delete longStats;
1000- feeStats = fileFeeStats.release();
1001- shortStats = fileShortStats.release();
1002- longStats = fileLongStats.release();
1003+ feeStats.reset(fileFeeStats.release());
How about
0feeStats = std::move(fileFeeStats);
1shortStats = std::move(fileShortStats);
2longStats = std::move(fileLongStats);
It would be nice to replace all the new
std::unique_ptr<ClassName>(new ClassName(...))
occurrences this is adding with simpler
make_unique<ClassName>(...)
calls.
std::make_unique isn’t available until c++14, so we can’t use it yet, but it is easy enough to write a substitute. For example (from #10973):
0//! Substitute for C++14 std::make_unique.
1template <typename T, typename... Args>
2std::unique_ptr<T> MakeUnique(Args&&... args)
3{
4 return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
5}
486@@ -487,7 +487,7 @@ bool CDB::Rewrite(CWalletDBWrapper& dbw, const char* pszSkip)
487 std::string strFileRes = strFile + ".rewrite";
488 { // surround usage of db with extra {}
489 CDB db(dbw, "r");
490- Db* pdbCopy = new Db(env->dbenv.get(), 0);
491+ std::unique_ptr<Db> pdbCopy = std::unique_ptr<Db>(new Db(env->dbenv.get(), 0));
In commit “Use unique_ptr for pdbCopy (Db)”
Is this fixing a potential resource / memory leak? It seems like the pointer was only being freed conditionally below. Maybe update commit description to say how this is changing behavior, if it is changing behavior.
49@@ -50,7 +50,7 @@ struct BasicTestingSetup {
50 */
51 class CConnman;
52 struct TestingSetup: public BasicTestingSetup {
53- CCoinsViewDB *pcoinsdbview;
54+ std::unique_ptr<CCoinsViewDB> pcoinsdbview;
In commit “Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree”
Can this variable just be deleted like the one in rpcnestedtests.h, since it is shadowing a global with the same name? If it can’t be deleted, it would be good to have a comment noting that it does shadow the global (and maybe explaining why).
Avoid resource leaks (specifically: forgetting to delete an object created using new)
Just want to add that this is not a panacea. In a few places I’ve explicitly used manual pointer allocation/deallocation instead of unique_ptr (for globals) so to explicitly control the lifespan, because the order of deallocation matters. So in some of these cases, forgetting explicit .reset() is as bad as leaking and the ‘parachute’ that automatic deallocation provides isn’t worth anything (could even result in spurious crashes at shutdown).
I like the concept, and encourage anyone who wants more meat on the bone to read Sutter’s Mill articles about smart pointers
@laanwj Absolutely true! My goal with this PR is to address only the trivial/safe cases :-)
I know about the dependency on deconstruction order related to LogPrintStr(…) (FILE* fileout, boost::mutex* mutexDebugLog and std::list<std::string>* vMsgsBeforeOpenLog) - so these were intentionally excluded. Were you thinking about to those when mentioning spurious crashes at shutdown? :-)
Were you thinking about to those when mentioning spurious crashes at shutdown? :-)
No, I’ll comment inline.
252@@ -253,8 +253,6 @@ void StopHTTPRPC()
253 LogPrint(BCLog::RPC, "Stopping HTTP RPC server\n");
254 UnregisterHTTPHandler("/", true);
255 if (httpRPCTimerInterface) {
256- RPCUnsetTimerInterface(httpRPCTimerInterface);
257- delete httpRPCTimerInterface;
258- httpRPCTimerInterface = 0;
259+ RPCUnsetTimerInterface(httpRPCTimerInterface.get());
httpRPCTimerInterface.reset() after this
29 WalletTestingSetup::~WalletTestingSetup()
30 {
31- UnregisterValidationInterface(pwalletMain);
32- delete pwalletMain;
33- pwalletMain = nullptr;
34+ UnregisterValidationInterface(pwalletMain.get());
pwalletMain.reset() (this is not part of the WalletTestingSetup but a global, so needs to be cleaned up here and now)
I found two in a cursory look-over. There might be more instances where explicit .reset is important (where there was an explicit delete before), please be careful in reviewing this.
LogPrintStr(…) (FILE* fileout, boost::mutex* mutexDebugLog and std::liststd::string* vMsgsBeforeOpenLog) so these were intentionally excluded
Agree with leaving these changes out for that “magic”, just too easy to inadvertently break something there.
Thanks for the re-review @ryanofsky!
Are we getting close to merge? :-)
Are we getting close to merge? :-)
We might want to wait until the 0.15.1 stuff is merged, to ease backports. Also, noting that some commits should be squashed before merge.
* pcoinscatcher (CCoinsViewErrorCatcher)
* pcoinsdbview (CCoinsViewDB)
* pcoinsTip (CCoinsViewCache)
* pblocktree (CBlockTreeDB)
* Remove variables shadowing pcoinsdbview
From @ryanofsky:s #10973. Thanks!