Make Get/Have in CCoinsView "const" #4561

domob1812 commented at 3:20 PM on July 19, 2014: contributor

This patch makes certain functions (Read/Exists in CLevelDBWrapper and diverse Get/Have ones in CCoinsView and subclasses) const. IMHO, this is the "right thing" for them, since they represent in fact "read-only access". This can also be used to make the CCoinsView reference in some higher-level functions (e. g., CheckInputs) const.

Is there a reason why this can't be done? It should make the code cleaner. Note that the three commits encapsulate "different levels" in the change here, and only the first two or only the first can be used, too, in case there is a good reason why the full patch isn't appropriate.

in src/coins.cpp:None in da2226a2e8 outdated

  98 | @@ -99,19 +99,29 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoins(const uint256 &txid) {
  99 |      return ret;
 100 |  }
 101 |  
 102 | +CCoinsMap::const_iterator CCoinsViewCache::FetchCoins(const uint256 &txid) const {
 103 | +    /* Avoid redundant implementation with the const-cast.  */
 104 | +    return const_cast<CCoinsViewCache*>(this)->FetchCoins(txid);

laanwj commented at 5:30 AM on July 20, 2014:

I don't like this construction.

domob1812 commented at 6:36 PM on July 20, 2014:

I understand your point. In this case, however, I believe that the "const" is justified - one could simply copy the implementation of the non-const FetchCoins and replace "iterator" by "const_iterator". I used the const_cast to avoid code duplication - but if you prefer, I can also implement it with a private template routine that can then be used for both the const and the non-const variant of FetchCoins. Would that be better?

laanwj commented at 12:07 PM on July 23, 2014:

Hm - I see the point now! A template-based common function would be slightly less evil, although if moving the implementation to the header pulls in extra dependencies, it may be best to keep it like this.

laanwj commented at 12:09 PM on July 23, 2014:

Maybe a better question: Do we need a non-const FetchCoins/GetCoins at all? Is the output ever modified? That'd be pretty ugly.

sipa commented at 12:14 PM on July 23, 2014:

Yes, all utxo updates are done that way to avoid copying.

laanwj commented at 12:17 PM on July 23, 2014:

Ok, right.

in src/coins.cpp:None in da2226a2e8 outdated

 110 |      return it->second;
 111 |  }
 112 |  
 113 | +const CCoins &CCoinsViewCache::GetCoins(const uint256 &txid) const {
 114 | +    /* Avoid redundant implementation with the const-cast.  */
 115 | +    return const_cast<CCoinsViewCache*>(this)->GetCoins(txid);

laanwj commented at 5:32 AM on July 20, 2014:

Same here, if you need a const cast that's an indication that something should not be const, or (in case of caching and such) that you need a 'mutable' field.

domob1812 commented at 6:37 PM on July 20, 2014:

In this case, I guess the best way forward is to simply duplicate the three lines of code from non-const GetCoins (as they are rather trivial). Is that ok?

sipa commented at 2:49 PM on July 27, 2014:

I'm fine with the const_cast here. It's a typical example of not propagating constness correctly automatically.

sipa commented at 2:50 PM on July 27, 2014: member

Needs rebase.

domob1812 commented at 6:03 PM on July 27, 2014: contributor

Done. Is it ok like this?

sipa commented at 12:10 PM on July 29, 2014: member

Untested ACK

jgarzik commented at 4:49 PM on July 29, 2014: contributor

Dislike the cast, but I don't have a better compromise.

ut ACK

domob1812 commented at 4:56 PM on July 29, 2014: contributor

If you want, I can provide the template formulation as alternative and you can choose which seems best. But I believe that the cast is probably the easiest (and also clearest) solution.

jgarzik commented at 5:04 PM on July 29, 2014: contributor

@domob1812 no don't waste your time

laanwj added the label Improvement on Jul 31, 2014

domob1812 commented at 11:56 AM on August 12, 2014: contributor

Any news on this, should I do some changes to the patch or is it ok as-is?

laanwj commented at 5:10 PM on August 12, 2014: member

IMO this is fine as-is.

Related: In https://github.com/laanwj/bitcoin/commit/245606c28b483755ec895bd39a6055db7c6c63f5 I've taken the approach of renaming the write-back variant of GetCoins to ModifyCoins. This makes the intent clear, and avoid the misunderstanding that I had above.

laanwj commented at 9:20 AM on August 26, 2014: member

Needs rebase after #4683

Mark LevelDB "Read" and "Exists" functions as const.

Mark the "Read" and "Exists" functions in CLevelDBWrapper as "const".
They do not change anything in the DB, by definition.

ffb4c210bc

Make appropriate getter-routines "const" in CCoinsView.

Mark the "Get"/"Have" routines in CCoinsView and subclasses as "const".

a3dc587a62

Use const CCoinsView's at some places.

At some places where it is possible (e. g., CheckInputs), use a const
version of CCoinsView instead of a non-const one.

d0867acb0e

domob1812 force-pushed on Aug 26, 2014

domob1812 commented at 10:02 AM on August 26, 2014: contributor

Done. Can this be merged now?

BitcoinPullTester commented at 10:16 AM on August 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4561_d0867acb0e07ac63f03dcc555387f24322e8799e/ 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 Aug 26, 2014

sipa closed this on Aug 26, 2014

sipa referenced this in commit 790911ff0a on Aug 26, 2014

domob1812 deleted the branch on Aug 26, 2014

MarcoFalke locked this on Sep 8, 2021