Disallow by-value copies of CAutoFile #4986

pull theuni wants to merge 2 commits into bitcoin:master from theuni:nofilecopy changing 5 files +13 −9

theuni commented at 11:26 PM on September 25, 2014: member

One might assume (as I did) that CAutoFile would be ref-counted so that a copied object would delay closing the underlying file until all copies have gone out of scope. Since that's not the case with CAutoFile, explicitly disable copying.
autofile: don't copy CAutoFile by value eee030f6bc
theuni force-pushed on Sep 25, 2014
in src/serialize.h:None in 485f4b7c4d outdated
```
1161 | @@ -1162,6 +1162,10 @@ class CDataStream
1162 |   */
1163 |  class CAutoFile
1164 |  {
1165 | +private:
1166 | +    // Disallow copies
1167 | +    CAutoFile(const CAutoFile& lhs);
```
Diapolo commented at 6:52 AM on September 26, 2014:

Nit/Question: Wouldn't that better be rhs?

theuni commented at 5:14 PM on September 26, 2014:

Heh, yes, thanks. Even better to just drop the param name since it's unused anyway. I'll make the change.
Diapolo commented at 6:53 AM on September 26, 2014: none

Seems to be a good idea and I'm also adding this into #4978.
Diapolo commented at 7:19 AM on September 26, 2014: none

@theuni Not related, but I'm asking myself, why nType and nVersion are not private? There are access functions e.g. SetType() and GetVersion() anyway. What do you think?
laanwj commented at 7:21 AM on September 26, 2014: member

Good good. ACK.
theuni commented at 5:19 PM on September 26, 2014: member

@Diapolo Looks like it, yea. I'd go further and make file private too. Without a virtual dtor and refcounting, a derived class would likely crash and burn.

autofile: Disallow by-value copies of CAutoFile

One might assume that CAutoFile would be ref-counted so that a copied object
would delay closing the underlying file until all copies have gone out of
scope. Since that's not the case with CAutoFile, explicitly disable copying.

6eb67b0ed2

sipa commented at 5:32 PM on September 26, 2014: member

utACK
theuni force-pushed on Sep 26, 2014
theuni commented at 6:07 PM on September 26, 2014: member

Updated to fix @Diapolo's nit. Should be good to go now.
BitcoinPullTester commented at 6:07 PM on September 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4986_6eb67b0ed2b350b772f7edb67aee1bcf09c91b0b/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa merged this on Sep 26, 2014
sipa closed this on Sep 26, 2014
sipa referenced this in commit 64cfaf891f on Sep 26, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors