Do not shadow member variables in serialization #8468

pull paveljanik wants to merge 1 commits into bitcoin:master from paveljanik:20160805_Wshadow_serialization changing 10 files +50 −50

paveljanik commented at 7:54 PM on August 5, 2016: contributor

This solves task 2 from #8105. serialize.h and nVersion, nType shadowing.
in src/net.h:None in b8701be4c5 outdated
```
 283 | @@ -284,7 +284,7 @@ class CBanEntry
 284 |      ADD_SERIALIZE_METHODS;
 285 |  
 286 |      template <typename Stream, typename Operation>
 287 | -    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
 288 | +    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int _nVersion) {
```
theuni commented at 9:15 PM on August 5, 2016:

I don't see what this was supposed to be doing before, but this look like a self-assign now.

paveljanik commented at 9:44 PM on August 5, 2016:

I do not understand the original code... :(

https://github.com/bitcoin/bitcoin/blob/master/src/net.h#L286-L293

The original code is assigning to the function argument already.

paveljanik commented at 7:53 AM on August 6, 2016:

@jonasschnelli Can you please explain the original code? It is part of your changes in #6310 (https://github.com/bitcoin/bitcoin/pull/6310/commits/409bccfbf52b531b2a9d60ac2308f56223931a2e). Thank you!

paveljanik commented at 8:14 AM on August 6, 2016:

The same code is in https://github.com/bitcoin/bitcoin/blob/master/src/qt/walletmodel.h#L68-L78 and https://github.com/bitcoin/bitcoin/blob/master/src/qt/recentrequeststablemodel.h#L29-L34.
jonasschnelli added the label Refactoring on Aug 8, 2016
sipa commented at 1:40 PM on September 1, 2016: member

What is the status here?
paveljanik commented at 2:03 PM on September 1, 2016: contributor

The change in src/net.h is unclear (at least to me ;-), even in the current tree. What is the purpose of the line https://github.com/bitcoin/bitcoin/blob/master/src/net.h#L291? Assigning to a shadowed variable which is not used at all down below this assignment is strange... It has no meaning in READ, it has no meaning in WRITE.

IIRC we had a short talk about this on IRC with @laanwj a few days ago.

Edit: I PRed its removal in #8658 as a test...
laanwj commented at 2:17 PM on September 1, 2016: member

I think the important requirement here, as for any variable shadowing change, is that the executables stay the same after renaming.
paveljanik force-pushed on Sep 23, 2016
paveljanik commented at 8:09 AM on September 23, 2016: contributor

Rebased. This is waiting for #8658 though, no need to review now.
paveljanik force-pushed on Sep 23, 2016
paveljanik force-pushed on Sep 23, 2016
paveljanik force-pushed on Sep 23, 2016
Do not shadow member variables in serialization 09a0658689
paveljanik force-pushed on Sep 30, 2016
paveljanik commented at 6:11 AM on September 30, 2016: contributor

Rebased.
MarcoFalke commented at 5:20 PM on September 30, 2016: member

Binaries are not the same

laanwj commented at 5:50 PM on September 30, 2016: member

Binaries are not the same

Indeed. Either with -O2 or without, there are binary differences. The following functions are affected:

void CBanEntry::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CBanEntry::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CAutoFile, CSerActionSerialize>(CAutoFile&, CSerActionSerialize, int, int)
void CBlockHeader::SerializationOp<CAutoFile, CSerActionUnserialize>(CAutoFile&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CBufferedFile, CSerActionUnserialize>(CBufferedFile&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CBlockHeader::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CBlockHeader::SerializationOp<CHashWriter, CSerActionSerialize>(CHashWriter&, CSerActionSerialize, int, int)
void CBlockHeader::SerializationOp<CSizeComputer, CSerActionSerialize>(CSizeComputer&, CSerActionSerialize, int, int)
void CHDChain::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CHDChain::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CKeyMetadata::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CKeyMetadata::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)

theuni commented at 4:23 PM on October 20, 2016: member

It appears as though the changes come from the fact that the passed variable is now unused. Presumably the compiler notices that and doesn't bother copying. @paveljanik Rather than passing _nVersion in the unused case, could you just leave the variable unnamed instead? That will also cut down on the "unused parameter" warnings, should we ever decide to enable those.
paveljanik commented at 4:31 PM on October 20, 2016: contributor

@theuni unfortunately nVersion is used in READWRITE(...). Right now, I do not know how to match the requirement to have the same binary...
sipa commented at 11:40 PM on October 29, 2016: member

I don't think it's feasible to get the same binary with this kind of changes. Given that, I have a more invasive change to the serialization code that removes the nType and nVersion being passed around entirely instead; see #9039.
paveljanik commented at 6:25 PM on October 30, 2016: contributor

@sipa 's approach is much better in general and also solves this issue, so closing this one.
paveljanik closed this on Oct 30, 2016
furszy referenced this in commit 8cd9c592a9 on May 29, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8105 avoid shadowing vars, and enable -Wshadow #8658 Remove unused statements in serialization #9039 Various serialization simplifcations and optimizations