Fix code constness in CBlockIndex::GetAncestor() overloads #11337

danra commented at 8:18 AM on September 15, 2017: contributor

Make the non-const overload of CBlockIndex::GetAncestor() reuse the const overload implementation instead of the other way around. This way, the constness of the const overload implementation is guaranteed. The other way around, it was possible to implement the non-const overload in a way which mutates the object, and since that implementation would be called even for const objects (due to the reuse), we would get undefined behavior.

Fix code constness in CBlockIndex::GetAncestor() overloads

Make the non-const overload of CBlockIndex::GetAncestor() reuse the const overload implementation instead of the other way around. This way, the constness of the const overload implementation is guaranteed. The other way around, it was possible to implement the non-const overload in a way which mutates the object, and since that implementation would be called even for const objects (due to the reuse), we would get undefined behavior.

b4058ed9c6

in src/chain.cpp:85 in b4058ed9c6

  79 | @@ -80,12 +80,13 @@ int static inline GetSkipHeight(int height) {
  80 |      return (height & 1) ? InvertLowestOne(InvertLowestOne(height - 1)) + 1 : InvertLowestOne(height);
  81 |  }
  82 |  
  83 | -CBlockIndex* CBlockIndex::GetAncestor(int height)
  84 | +const CBlockIndex* CBlockIndex::GetAncestor(int height) const
  85 |  {
  86 | -    if (height > nHeight || height < 0)
  87 | +    if (height > nHeight || height < 0) {

promag commented at 9:22 AM on September 15, 2017:

Nit, kind of unrelated since this is not touched.

danra commented at 9:33 AM on September 15, 2017:

True, but from what I've seen so far it's much easier to get style improvements (even those following the Developer Notes) approved in pull requests which actually also change functionality. Usually I would do that in a separate commit, but this is such a small change that I thought it made sense to just add it to the single commit.

promag commented at 9:24 AM on September 15, 2017: member

Agree this is better. utACK b4058ed.

laanwj added the label Refactoring on Sep 15, 2017

in src/chain.cpp:112 in b4058ed9c6

 106 | @@ -106,9 +107,9 @@ CBlockIndex* CBlockIndex::GetAncestor(int height)
 107 |      return pindexWalk;
 108 |  }
 109 |  
 110 | -const CBlockIndex* CBlockIndex::GetAncestor(int height) const
 111 | +CBlockIndex* CBlockIndex::GetAncestor(int height)
 112 |  {
 113 | -    return const_cast<CBlockIndex*>(this)->GetAncestor(height);
 114 | +    return const_cast<CBlockIndex*>(static_cast<const CBlockIndex*>(this)->GetAncestor(height));

TheBlueMatt commented at 7:12 PM on September 15, 2017:

Shouldnt the new cast also be a const_cast?

danra commented at 9:05 PM on September 15, 2017:

Both static_cast and const_cast work for casting an object of a type to an object of the same type with added const. Personally, I prefer static_cast for that case, since const_cast, when encountered in code, always has to be examined for its more dangerous use case - removing constness.

sipa commented at 11:27 PM on September 15, 2017:

Interesting, I wasn't aware, but makes perfect sense.

TheBlueMatt commented at 7:52 PM on September 19, 2017:

Gah, but static_cast could do something crazy like call a constructor or create a temporary. In this case its not like a reader is confused if the resulting type will be const or not.

danra commented at 5:47 PM on September 28, 2017:

Indeed. So both types of casts here have some disadvantage. I went to have a look at what the C++ Core Guidelines have to say on the matter, and found the exact same usage (with static_cast) in their example here: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#example-219

However they list an even better way to do it, which I am going to amend this pull request with :)

TheBlueMatt commented at 5:52 PM on September 28, 2017:

Gah holy shit lets not use their insane unreadable suggestion there, please. I'd prefer static_cast or const_cast over that.

danra commented at 6:03 PM on September 28, 2017:

Oh dear, that's the second 'Gah' I've elicited in a single thread :)

TheBlueMatt commented at 6:06 PM on September 28, 2017:

Heh, I mean I'm OK with static_cast, its not a huge deal...I find const_cast more readable, but its somewhat obvious. The CppCore guidelines there are unreadable garbage, IMO, and would be a step back from the original version.

danra commented at 6:13 PM on September 28, 2017:

The author of the preferred snippet there is Herb Sutter (commit 2d8377aab56354f7fc9bf759ab134c983a10293e). So in an appeal to authority, I suggest we do use that proposed solution, rather than the multiple casts solution.

Personally, I also think it's a better solution.

Need more opinions here.

TheBlueMatt commented at 11:28 PM on September 28, 2017:

Please, lets not add a template, decltype gook and a private function just to avoid an obvious const_cast, its not only harder to read but its another function call away instead of direct. Maybe its "the correct way" in that it avoids some casts and magically deduces what you felt like doing, but magic also makes it unreadable to determine exactly what something does instead of what it looks like its author probably intended it to do any maybe does.

sipa commented at 11:28 PM on September 15, 2017: member

utACK b4058ed9c6e1f23720afa0b383c56a9aaed86dcd. I think the nearby style change is fine.

practicalswift commented at 6:33 PM on September 16, 2017: contributor

utACK b4058ed9c6e1f23720afa0b383c56a9aaed86dcd

laanwj commented at 2:19 PM on December 1, 2017: member

agree it's slightly better utACK b4058ed

laanwj merged this on Dec 1, 2017

laanwj closed this on Dec 1, 2017

laanwj referenced this in commit 0d7e0a3289 on Dec 1, 2017

markblundeberg referenced this in commit 3476d45ba1 on Jun 5, 2019

jtoomim referenced this in commit de56888274 on Jun 29, 2019

jonspock referenced this in commit 9a7d867667 on Jul 4, 2019

jonspock referenced this in commit 2c1056d027 on Jul 4, 2019

proteanx referenced this in commit 4d52f0671d on Jul 5, 2019

jonspock referenced this in commit 0b4a16fa7a on Jul 9, 2019

PastaPastaPasta referenced this in commit ead817d777 on Jan 17, 2020

PastaPastaPasta referenced this in commit e99e55d34d on Jan 22, 2020

PastaPastaPasta referenced this in commit 883d22d3cc on Jan 22, 2020

PastaPastaPasta referenced this in commit 27369699e3 on Jan 29, 2020

PastaPastaPasta referenced this in commit 2726efd194 on Jan 29, 2020

PastaPastaPasta referenced this in commit dac133ed8d on Jan 29, 2020

PastaPastaPasta referenced this in commit 48ed520f10 on Jan 30, 2020

ckti referenced this in commit a489efabf5 on Mar 28, 2021

furszy referenced this in commit 60d36292bc on Jul 26, 2021

DrahtBot locked this on Sep 8, 2021