Do not shadow variables (gcc set)

paveljanik commented at 5:22 pm on September 25, 2016: contributor

Followup to #8105.

The current set of PRs made clang compile Wshadow clean. Compiling with gcc revealed new needed changes. gcc is more strict in warnings. This is non-Qt part. I do not have an environment to test gcc+Qt build now.

paveljanik force-pushed on Sep 25, 2016

paveljanik commented at 7:08 pm on September 25, 2016: contributor

Hmm, two failures in p2p-segwit.py. Is there some issue with this test script?

dcousens approved

MarcoFalke commented at 7:27 am on September 26, 2016: member

Travis fails reliably. Is the commit producing identical binaries?

paveljanik commented at 3:25 pm on September 26, 2016: contributor

This commit should produce same binaries, yes.

in src/primitives/block.h: in df9b4a51f2 outdated

91@@ -92,7 +92,7 @@ class CBlock : public CBlockHeader
92     ADD_SERIALIZE_METHODS;
93 
94     template <typename Stream, typename Operation>
95-    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int _nVersion) {
96+    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {

paveljanik commented at 7:54 pm on September 26, 2016:

Hmm, this particular change is causing my clang on OS X build to fail -rescan with:

02016-09-26 19:53:08 ERROR: ReadBlockFromDisk: Deserialize or I/O error - ReadCompactSize(): size too large: unspecified iostream_category error at CBlockDiskPos(nFile=49, nPos=4208853)

So reverting it.

But do not ask me, why it is the cause… I do not know yet.

laanwj commented at 8:04 am on September 28, 2016:

I think I figured this out, see #8658 (comment) and the comment below that.

laanwj commented at 8:16 am on September 28, 2016: member

This introduces binary changes in these serialization functions:

0void CDiskBlockIndex::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
1void CDiskBlockIndex::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
2void CDiskBlockIndex::SerializationOp<CSizeComputer, CSerActionSerialize>(CSizeComputer&, CSerActionSerialize, int, int)
3void CMerkleTx::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
4void CMerkleTx::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
5void CWalletTx::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
6void CWalletTx::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)

Edit: and in contrast to #8658, these do not go away with -O2

laanwj added the label Refactoring on Sep 30, 2016

paveljanik force-pushed on Oct 2, 2016

paveljanik commented at 5:44 pm on October 2, 2016: contributor

OK, rebased and removed problematic parts (all serialization related, nVersion)…

paveljanik force-pushed on Oct 7, 2016

paveljanik commented at 2:17 pm on October 7, 2016: contributor

Rebased.

paveljanik commented at 12:06 pm on November 7, 2016: contributor

@MarcoFalke Marco, can you please test for the same binaries here?

MarcoFalke commented at 1:03 pm on November 7, 2016: member

There seems to be a small diff:

0319294c319294
1<   17d0f1: lea    0x2dd1b2(%rip),%rdi        # 45a2aa <leveldb::Status::Status(leveldb::Status::Code, leveldb::Slice const&, leveldb::Slice const&)::__PRETTY_FUNCTION__+0xaa>
2---
3>   17d0f1: lea    0x2dd1b1(%rip),%rdi        # 45a2a9 <leveldb::Status::Status(leveldb::Status::Code, leveldb::Slice const&, leveldb::Slice const&)::__PRETTY_FUNCTION__+0xa9>

used the following command:

0commit=416654b4bdcc9f91cf84d4e3c645826d62979c1d && git checkout bitcoin/master && git reset --hard HEAD && curl https://raw.githubusercontent.com/laanwj/bitcoin-maintainer-tools/6e4425587736144b067f67ad792d9ee904e74fd7/patches/stripbuildinfo.patch | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_old && curl https://github.com/bitcoin/bitcoin/commit/"${commit}".diff | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_new && diff /tmp/d_old /tmp/d_new | wc

paveljanik commented at 1:28 pm on November 7, 2016: contributor

This seems unrelated.

fanquake commented at 2:12 pm on November 9, 2016: member

I haven’t done the identical binary checking yet, but it looks like the difference could be here ? 170 (0xaa) vs 169 (0xa9)

paveljanik commented at 2:17 pm on November 9, 2016: contributor

@fanquake surely not…

paveljanik commented at 2:26 pm on November 9, 2016: contributor

JFYI: I’m now building with other gcc version and there are a few other warnings 8)

in src/arith_uint256.cpp: in 416654b4bd outdated

172@@ -173,9 +173,9 @@ unsigned int base_uint<BITS>::bits() const
173 {
174     for (int pos = WIDTH - 1; pos >= 0; pos--) {
175         if (pn[pos]) {
176-            for (int bits = 31; bits > 0; bits--) {

gmaxwell commented at 9:45 pm on November 9, 2016:

change _bits to nbits.

paveljanik commented at 6:36 am on November 10, 2016:

Will do.

paveljanik commented at 10:59 am on November 10, 2016:

Done

in src/streams.h: in 416654b4bd outdated

528+        size_t _read = fread((void*)&vchBuf[pos], 1, readNow, src);
529+        if (_read == 0) {
530             throw std::ios_base::failure(feof(src) ? "CBufferedFile::Fill: end of file" : "CBufferedFile::Fill: fread failed");
531         } else {
532-            nSrcPos += read;
533+            nSrcPos += _read;

gmaxwell commented at 9:46 pm on November 9, 2016:

change _read to nBytes

paveljanik commented at 6:36 am on November 10, 2016:

Will do.

paveljanik commented at 11:00 am on November 10, 2016:

Done

MarcoFalke commented at 9:47 pm on November 9, 2016: member

Concept ACK when this is the last patch/pull with regard to Wshadow.

in src/timedata.h: in 416654b4bd outdated

47@@ -48,14 +48,14 @@ class CMedianFilter
48 
49     T median() const
50     {
51-        int size = vSorted.size();
52-        assert(size > 0);
53-        if (size & 1) // Odd number of elements
54+        int _size = vSorted.size();

gmaxwell commented at 9:48 pm on November 9, 2016:

vecsize

dcousens commented at 1:41 am on November 10, 2016:

vSortedSize ?

paveljanik commented at 6:36 am on November 10, 2016:

I like vSortedSize, will do.

in src/primitives/transaction.h: in 416654b4bd outdated

292@@ -293,7 +293,7 @@ inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action, in
293     const bool fAllowWitness = !(nVersion & SERIALIZE_TRANSACTION_NO_WITNESS);
294 
295     READWRITE(*const_cast<int32_t*>(&tx.nVersion));
296-    unsigned char flags = 0;
297+    unsigned char _flags = 0;

gmaxwell commented at 9:51 pm on November 9, 2016:

sflags (this was less obvious to me, but my suggestion and @sipa thought it was okay)

dcousens commented at 1:41 am on November 10, 2016:

serializeFlags?

paveljanik commented at 6:55 am on November 10, 2016:

I like both.

in src/wallet/wallet.cpp: in 416654b4bd outdated

279@@ -280,17 +280,17 @@ bool CWallet::LoadWatchOnly(const CScript &dest)
280 bool CWallet::Unlock(const SecureString& strWalletPassphrase)
281 {
282     CCrypter crypter;
283-    CKeyingMaterial vMasterKey;
284+    CKeyingMaterial _vMasterKey;

gmaxwell commented at 9:57 pm on November 9, 2016:

So I believe changing the name of CCryptoKeyStore master key will, I think change only four lines. Might be good to get an opinion from someone who’s worked more on this code? @TheBlueMatt @pstratem

paveljanik commented at 6:56 am on November 10, 2016:

That would be great and even better solution, yes. Will wait.

gmaxwell commented at 10:01 pm on November 9, 2016: contributor

filling up locals with _ prefixes in big pieces of code is less good than using better names, and also incompatible with a common convention of using _prefixes for function parameters.

gmaxwell commented at 10:10 pm on November 9, 2016: contributor

I checked each of the changes so far, they appear correct to me (+/- the suggestion I made about changing the name of the member vMasterKey that I asked for others input on). I didn’t check that they resulted in the same code.

paveljanik force-pushed on Nov 10, 2016

paveljanik commented at 7:02 am on November 10, 2016: contributor

Rebased, updated.

It is now -Wshadow clean for both old gcc (tested on gcc version 4.8.3 20140627 [gcc-4_8-branch revision 212064](SUSE Linux)) and also on the ultimate gcc (gcc version 6.2.1 20160830 [gcc-6-branch revision 239856](SUSE Linux)). And of course stays clean on other systems I regularly test on.

MarcoFalke commented at 10:35 am on November 10, 2016: member

@paveljanik Mind to cherry-pick fd5654c, so we don’t have to open another pull?

MarcoFalke commented at 10:37 am on November 10, 2016: member

I get same binaries for bedf9a7 besides this diff:

0commit=bedf9a7 && git checkout bitcoin/master && git reset --hard HEAD && curl https://raw.githubusercontent.com/laanwj/bitcoin-maintainer-tools/6e4425587736144b067f67ad792d9ee904e74fd7/patches/stripbuildinfo.patch | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_old && curl https://github.com/bitcoin/bitcoin/commit/"${commit}".diff | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_new && diff /tmp/d_old /tmp/d_new

 0317267c317267
 1<   17aa0b: lea    0x25704b(%rip),%rsi        # 3d1a5d <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0xfd>
 2---
 3>   17aa0b: lea    0x25705b(%rip),%rsi        # 3d1a6d <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0x10d>
 4317684c317684
 5<   17b325: lea    0x256737(%rip),%rsi        # 3d1a63 <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0x103>
 6---
 7>   17b325: lea    0x256747(%rip),%rsi        # 3d1a73 <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0x113>
 8317757c317757
 9<   17b471: lea    0x2dc552(%rip),%rdi        # 4579ca <leveldb::Status::Status(leveldb::Status::Code, leveldb::Slice const&, leveldb::Slice const&)::__PRETTY_FUNCTION__+0xaa>
10---
11>   17b471: lea    0x2565e5(%rip),%rdi        # 3d1a5d <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0xfd>

paveljanik commented at 10:37 am on November 10, 2016: contributor

@MarcoFalke I can, but Wladimir doesn’t want it to be enabled by default…

MarcoFalke commented at 11:17 am on November 11, 2016: member

utACK f14745d

Either the warning is enabled and people can fix the warnings when they write new code or there is no warning and we stop caring about it. There is no such thing as “disable the warnign by default and create refactoring pulls every other week to fix new occurences”.

To conclude, I think this is ready for merge. If it turns out to cause problems in the near future, we disable it and call it done.

paveljanik commented at 8:59 am on November 26, 2016: contributor

@MarcoFalke Agreed. We should merge this and then fix the remaining issues found by users who use other build environments/configurations.

jtimon commented at 7:12 pm on November 27, 2016: contributor

Concept ACK

paveljanik commented at 10:38 am on December 5, 2016: contributor

Working on needed rebase…

Do not shadow variables (gcc set) 9de90bb749

Check and enable -Wshadow by default. ad1ae7ae2e

paveljanik force-pushed on Dec 5, 2016

MarcoFalke commented at 8:41 pm on January 26, 2017: member

utACK ad1ae7ae2e90a29fcfaaddee2d5acd025de72cbe, 9de90bb gives same binaries for me besides some boost_pretty_functions.

jtimon commented at 1:11 am on January 27, 2017: contributor

utACK ad1ae7a

laanwj commented at 2:48 pm on March 3, 2017: member

Checked that this, rebased on master, results in identical bitcoind executables. ACK ad1ae7a

laanwj merged this on Mar 3, 2017

laanwj closed this on Mar 3, 2017

laanwj referenced this in commit 75d012e8c7 on Mar 3, 2017

laanwj referenced this in commit 2c83911401 on Apr 1, 2017

practicalswift referenced this in commit c8afd888d2 on Apr 27, 2017

PastaPastaPasta referenced this in commit d659f91b70 on Dec 30, 2018

PastaPastaPasta referenced this in commit fdaf71f6db on Dec 30, 2018

PastaPastaPasta referenced this in commit cbd924589a on Dec 30, 2018

PastaPastaPasta referenced this in commit b092cafbbf on Dec 31, 2018

PastaPastaPasta referenced this in commit b5d945f726 on Dec 31, 2018

PastaPastaPasta referenced this in commit c8d74b3c8f on Dec 31, 2018

PastaPastaPasta referenced this in commit 9531c0d227 on Dec 31, 2018

PastaPastaPasta referenced this in commit db7fcf4444 on Dec 31, 2018

PastaPastaPasta referenced this in commit 5a3d30a6ce on Dec 31, 2018

PastaPastaPasta referenced this in commit 9337a6f32b on Dec 31, 2018

PastaPastaPasta referenced this in commit 9664e62274 on Jan 2, 2019

PastaPastaPasta referenced this in commit 4fcbf7d48f on Jan 2, 2019

PastaPastaPasta referenced this in commit 8ee0fefcbb on Jan 2, 2019

PastaPastaPasta referenced this in commit 07bbdd08db on Jan 2, 2019

PastaPastaPasta referenced this in commit bb31372aa4 on Jan 3, 2019

PastaPastaPasta referenced this in commit 074f5e577e on Jan 3, 2019

PastaPastaPasta referenced this in commit 500c72057f on Jan 21, 2019

PastaPastaPasta referenced this in commit 1f764bcba7 on Jan 21, 2019

PastaPastaPasta referenced this in commit 706c56ebb8 on Jan 25, 2019

PastaPastaPasta referenced this in commit 7ec4cd4f0f on Jan 25, 2019

PastaPastaPasta referenced this in commit f3a72e52fe on Jan 29, 2019

PastaPastaPasta referenced this in commit 6797b96d46 on Jan 29, 2019

PastaPastaPasta referenced this in commit fc3bdaca9c on Feb 1, 2019

PastaPastaPasta referenced this in commit 9bbf67fddb on Feb 1, 2019

PastaPastaPasta referenced this in commit ead4559b99 on Feb 1, 2019

PastaPastaPasta referenced this in commit 3d3443b6a9 on Feb 1, 2019

MarcoFalke locked this on Sep 8, 2021

Do not shadow variables (gcc set) #8808