Wshadow: various gcc fixes

paveljanik commented at 5:37 pm on March 3, 2017: contributor

Additional -Wshadow fixes for gcc 4.8.5.

paveljanik renamed this:
~~WIP: Wshadow: gcc 4.8.5 fixes~~
WIP: Wshadow: various gcc fixes
on Mar 3, 2017

paveljanik commented at 6:14 pm on March 3, 2017: contributor

gcc version 5.3.1 20160301 [gcc-5-branch revision 233849] is completely -Wshadow clean. The same applies to gcc version 6.2.1 20160826 [gcc-6-branch revision 239773].

fanquake added the label Refactoring on Mar 3, 2017

gmaxwell commented at 7:40 pm on March 4, 2017: contributor

utACK (lightly reviewed)

MarcoFalke commented at 8:02 pm on March 4, 2017: member

Any reason this is split into three commits with mostly the same commit messages?

On Sat, Mar 4, 2017 at 8:40 PM, Gregory Maxwell notifications@github.com wrote:

@gmaxwell commented on this pull request.

utACK

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

paveljanik commented at 8:20 pm on March 4, 2017: contributor

Yes. It is WIP. I’m adding commit by commit for various gcc/clang version I test so people do not see warnings. I still have to test one more clang version I have installed on one system.

Wil squash afterwards of course.

paveljanik commented at 8:42 pm on March 4, 2017: contributor

BTW, this PR is open also for others who see some -Wshadow warnings on their preferred compiler. Having all fixes in one PR should lower the impact of turning the warnings on by default.

laanwj commented at 9:32 pm on March 4, 2017: member

Sigh, more Wshadow changes. Is this ever done?

paveljanik force-pushed on Mar 4, 2017

paveljanik commented at 9:59 pm on March 4, 2017: contributor

… and squashed as both clang version tested are -Wshadow clean.

clang seems to be much better or people use it more when developing for Bitcoin Core ;-)

paveljanik renamed this:
~~WIP: Wshadow: various gcc fixes~~
Wshadow: various gcc fixes
on Mar 4, 2017

laanwj commented at 10:21 am on March 5, 2017: member

clang seems to be much better or people use it more when developing for Bitcoin Core ;-)

Aside: In my case, yes. FreeBSD defaults to clang nowadays, and on Linux I use clang specifically for Bitcoin Core because it compiles faster and uses less memory (=more parallelism with same amount of RAM).

GCC is supposed to produce slightly better code for x86 though, or at least that used to be the case years ago. So for production builds it makes sense to use that (which we do in gitian). I think @theuni was discussing switching all the builds to clang at some point though because it makes cross-compiling easier (also clang support for Win32 is supposed to be fairly good, whereas GCC is giving us a lot of trouble recently.) But it’s not possible to say generally that one of them is better.

dcousens approved

dcousens commented at 12:46 pm on March 5, 2017: contributor

utACK

paveljanik force-pushed on Mar 6, 2017

paveljanik commented at 4:24 pm on March 6, 2017: contributor

Added fix for #8574’s local variable redeclaration (aka shadowing).

For easier searching, here is the log:

0wallet/db.cpp:620:40: warning: declaration shadows a local variable [-Wshadow]
1            map<string, int>::iterator mi = bitdb.mapFileUseCount.find(strFile);
2                                       ^
3wallet/db.cpp:610:36: note: previous declaration is here
4        map<string, int>::iterator mi = bitdb.mapFileUseCount.begin();
5                                   ^
61 warning generated.

gmaxwell commented at 11:25 pm on March 6, 2017: contributor

paveljanik have you compiled with a make clean lately? I’m flooded with streams.h:407:44: warning: declaration of ‘data’ shadows a member of ’this’ [-Wshadow]

paveljanik commented at 5:49 am on March 7, 2017: contributor

@gmaxwell This should be already fixed in this PR - https://github.com/bitcoin/bitcoin/pull/9911/files#diff-eb3b977a68473a9d20093cbe90c659e6L407

BTW - I always clean compile…

jtimon commented at 5:23 pm on March 7, 2017: contributor

utACK bfc6be9

in src/torcontrol.cpp: in 6cff584f4f outdated

661@@ -662,26 +662,26 @@ void TorController::reconnect_cb(evutil_socket_t fd, short what, void *arg)
662 }
663 
664 /****** Thread ********/
665-struct event_base *base;

TheBlueMatt commented at 8:40 pm on March 8, 2017:

What is this shadowing? (and can we make it static?)

paveljanik commented at 8:51 pm on March 8, 2017:

With gcc4.8.5:

 0bitcoin@linux-5bz0:~/bitcoin> make CC=gcc
 1Making all in src
 2make[1]: Entering directory '/home/bitcoin/bitcoin/src'
 3make[2]: Entering directory '/home/bitcoin/bitcoin/src'
 4  CXX      libbitcoin_server_a-torcontrol.o
 5/usr/include/boost/date_time/time_system_counted.hpp: In instantiation of 'static boost::date_time::counted_time_system<time_rep>::time_rep_type boost::date_time::counted_time_system<time_rep>::add_days(const time_rep_type&, const date_duration_type&) [with time_rep = boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config>; boost::date_time::counted_time_system<time_rep>::time_rep_type = boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config>; boost::date_time::counted_time_system<time_rep>::date_duration_type = boost::gregorian::date_duration]':
 6/usr/include/boost/date_time/time.hpp:141:45:   required from 'boost::date_time::base_time<T, time_system>::time_type boost::date_time::base_time<T, time_system>::operator+(const date_duration_type&) const [with T = boost::posix_time::ptime; time_system = boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> >; boost::date_time::base_time<T, time_system>::time_type = boost::posix_time::ptime; boost::date_time::base_time<T, time_system>::date_duration_type = boost::gregorian::date_duration]'
 7/usr/include/boost/date_time/posix_time/date_duration_operators.hpp:33:37:   required from here
 8torcontrol.cpp:665:20: warning: shadowed declaration is here [-Wshadow]
 9 struct event_base *base;
10                    ^
11/usr/include/boost/date_time/time_system_counted.hpp: In instantiation of 'static boost::date_time::counted_time_system<time_rep>::time_rep_type boost::date_time::counted_time_system<time_rep>::add_time_duration(const time_rep_type&, boost::date_time::counted_time_system<time_rep>::time_duration_type) [with time_rep = boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config>; boost::date_time::counted_time_system<time_rep>::time_rep_type = boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config>; boost::date_time::counted_time_system<time_rep>::time_duration_type = boost::posix_time::time_duration]':
12/usr/include/boost/date_time/time.hpp:161:64:   required from 'boost::date_time::base_time<T, time_system>::time_type boost::date_time::base_time<T, time_system>::operator+(const time_duration_type&) const [with T = boost::posix_time::ptime; time_system = boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> >; boost::date_time::base_time<T, time_system>::time_type = boost::posix_time::ptime; boost::date_time::base_time<T, time_system>::time_duration_type = boost::posix_time::time_duration]'
13/usr/include/boost/date_time/posix_time/conversion.hpp:30:48:   required from here
14torcontrol.cpp:665:20: warning: shadowed declaration is here [-Wshadow]

TheBlueMatt commented at 9:00 pm on March 8, 2017:

Hmm, that doesnt really answer my question, thanks gcc…Is that trying to say that boost has some “base” object that we’re shadowing? Either way lets make the new gBase static.

paveljanik commented at 9:00 pm on March 8, 2017:

Hmm. I do not see any base there 8)

laanwj commented at 9:10 pm on March 8, 2017:

Oh no, so we’re at renaming random variables now without even understanding why just to shut up warnings.

paveljanik commented at 9:25 pm on March 8, 2017:

You are overreacting. The problem is elsewhere, of course. The original author is using base all over the source code and arguments of methods conflicts with the global (now static). I think that renaming the global and making it static is the best solution here. Yes, discovered because of turning on -Wshadow.

laanwj commented at 9:15 pm on March 8, 2017: member

To be honest I’d prefer to disable -Wshadow again. Even a few days after enabling it, it seems to me it requires too many disjointed, seemingly random changes, and continuous maintenance after almost every thing that gets merged after new incomprehensible warnings pop up with some gcc or clang version.

MarcoFalke commented at 9:31 pm on March 8, 2017: member

We might keep it enabled, after all it could help to detect valid bugs. Though, I agree that those “fix various gcc warnings” with no apparent rationale other than “The warning disappears when compiling with this specific version of boost and gcc-x.x.x” are not particularly helpful. (I am not against all changes in this pull, but we should only commit the fixes that make sense and keep the commits itself at a frequency that does not hinder the other development work.)

On Wed, Mar 8, 2017 at 10:15 PM, Wladimir J. van der Laan notifications@github.com wrote:

To be honest I’d prefer to disable -Wshadow again. Even a few days after enabling it, it seems to me it requires too many disjointed, seemingly random changes, and continuous maintenance after almost every thing that gets merged after new incomprehensible warnings pop up with some gcc or clang version.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

laanwj commented at 9:43 pm on March 8, 2017: member

We might keep it enabled, after all it could help to detect valid bugs.

It could, sure. In an ideal situation where compilers had a single, sane (or even close) definition of shadowing I’d be all for it. However I just searched and there are >25 recent pulls already dealing with Wshadow changes: https://github.com/bitcoin/bitcoin/pulls?utf8=%E2%9C%93&q=is%3Apr%20is%3Aclosed%20wshadow This is becoming a bit too much. And these kind of changes could also result in bugs when applied carelessly, e.g. forgetting to check binaries for equality on some platform.

Also changes in boost or other dependencies could suddenly trigger these in our code. E.g. on the version of boost on FreeBSD I see various warnings inside boost too.

laanwj commented at 7:34 am on March 9, 2017: member

@gmaxwell noted that there are also sub-options for Wshadow. Maybe we can reduce the false positives/weird cases this way.

0  -Wshadow-local which warns if a local variable shadows another local
1  variable or parameter,
2
3  -Wshadow-compatible-local which warns if a local variable shadows
4  another local variable or parameter whose type is compatible with that
5  of the shadowing variable.

paveljanik commented at 7:38 am on March 9, 2017: contributor

We had some false positive/weird case?

The base case is a gcc bug (edit: 4.8.x only; fixed in gcc 5). It correctly identified shadowing but emitted wrong log lines…

paveljanik force-pushed on Mar 9, 2017

paveljanik commented at 7:41 am on March 9, 2017: contributor

Rebased.

paveljanik commented at 8:22 am on March 9, 2017: contributor

@laanwj Can you please upload your FreeBSD build log for me with V=1 so I can investigate your warnings?

paveljanik force-pushed on Mar 15, 2017

paveljanik commented at 2:43 pm on March 15, 2017: contributor

Added fix for #9497’s shadowing issues:

 0test/checkqueue_tests.cpp:49:23: warning: declaration shadows a field of 'checkqueue_tests::FailingCheck' [-Wshadow]
 1    FailingCheck(bool fails) : fails(fails){};
 2                      ^
 3test/checkqueue_tests.cpp:48:10: note: previous declaration is here
 4    bool fails;
 5         ^
 6test/checkqueue_tests.cpp:414:50: warning: declaration shadows a local variable [-Wshadow]
 7                    std::unique_lock<std::mutex> l(m);
 8                                                 ^
 9test/checkqueue_tests.cpp:411:42: note: previous declaration is here
10            std::unique_lock<std::mutex> l(m);
11                                         ^
122 warnings generated.

laanwj commented at 10:12 am on March 17, 2017: member

01201f4dac236eb92dade95ae8dcb832b374c38463e1180177433b7c500dd56a7  /tmp/compare/bitcoind.0cebfcffb480e63527c225bf9c46db06a3816d98.stripped
11201f4dac236eb92dade95ae8dcb832b374c38463e1180177433b7c500dd56a7  /tmp/compare/bitcoind.58e94bf6bb3981bca466ff83994ca2a06630a828.stripped
21201f4dac236eb92dade95ae8dcb832b374c38463e1180177433b7c500dd56a7  /tmp/compare/bitcoind.d9f7224cde0aa9415efd945bc1c9124628a59002.stripped
3e813793f57842ece8ec04d101fb202a30414c3a6d6801b3038884e5efeb8d437  /tmp/compare/bitcoind.946ae52d0361058a1f5e70d48d6bec191217cf4d.stripped
4e813793f57842ece8ec04d101fb202a30414c3a6d6801b3038884e5efeb8d437  /tmp/compare/bitcoind.ce01e6226ce52b88c644b0cb89852278edbdf33b.stripped

Looks like d9f7224cde0aa9415efd945bc1c9124628a59002 changes the bitcoind executable. ACK on the others, if you make a pull with just those I’ll merge it.

paveljanik commented at 10:23 am on March 17, 2017: contributor

@laanwj Please look at d9f7224cde0aa9415efd945bc1c9124628a59002 ;-) Of course I can rewrite it by renaming the local variable (e.g. mi --> mIter?) instead of reusing the upper local which is not used after at all.

Is it OK for you? I have chosen this solution because it is crystal clean, logical, less diff lines, easier review etc.

MarcoFalke commented at 2:35 pm on March 17, 2017: member

Let’s not reuse iterators. This will cause issues with nested for loops, where the inner loops uses the iterator of the outer loop. I’d rather prefer to keep it the same instead of changing one issue against another.

paveljanik commented at 4:24 pm on March 17, 2017: contributor

@MarcoFalke Can you please rephrase what do you prefer? Do you agree with renaming the first or the second mi to e.g. mIter? Or mj? ;-)

MarcoFalke commented at 6:21 pm on March 17, 2017: member

Change one of them to mit?

Imo, what will cause problems when reusing iterators:

0for ( auto it = ... ++it ) {
1  for ( it = ... ++it ) {
2    do(*it);
3  }
4  do(*it);
5}

laanwj commented at 9:55 pm on March 17, 2017: member

I’m not going to merge any shadowing fixes if they change the binary.

In this case I’d prefer if you did the renaming in a way that just renames one of the iterators instead of reusing it which changes the code (thanks MarcoFalke for checking)

paveljanik commented at 6:21 am on March 18, 2017: contributor

@MarcoFalke Yes, but your example is not the code which is used right now (and this was the reason I did not understand what you wrote). There are two sequential uses, there is no loop in the second part and no nested loop at all. But yes, to be future-safe and same-binary-now, I have changed the first iterator name to mit in an additional commit for easier checking.

Prevent -Wshadow warnings with gcc versions 4.8.5, 5.3.1 and 6.2.1. bb2aaeeeea

Make some global variables less-global (static) c4b60b3d9c

Fix shadowing of local variables. b42ff60c7e

Rename first iterator to prevent shadowing. d7f80b6dcb

paveljanik force-pushed on Mar 18, 2017

laanwj commented at 9:16 am on March 18, 2017: member

@paveljanik thanks, going to re-check.

There are two sequential uses, there is no loop in the second part and no nested loop at all.

You are right, it’s just that in the interest of reducing friction (efficient checking) and eliminating the chance of accidental bugs introduced in these pulls we should require identical binaries.

laanwj merged this on Mar 18, 2017

laanwj closed this on Mar 18, 2017

laanwj referenced this in commit baae3149d6 on Mar 18, 2017

practicalswift referenced this in commit 5df7e68a3e on Mar 18, 2017

zkbot referenced this in commit 45faa928ec on Mar 26, 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 857c81aea0 on Mar 10, 2019

PastaPastaPasta referenced this in commit 93f2ff7f23 on Mar 11, 2019

PastaPastaPasta referenced this in commit ddd9c04a3b on Mar 11, 2019

PastaPastaPasta referenced this in commit b1a9b0d4f6 on Mar 12, 2019

PastaPastaPasta referenced this in commit 646812bfe9 on Mar 13, 2019

UdjinM6 referenced this in commit 45dcd467ca on Mar 13, 2019

PastaPastaPasta referenced this in commit e3d896e6a4 on Mar 13, 2019

PastaPastaPasta referenced this in commit 8116b8e19d on Mar 14, 2019

PastaPastaPasta referenced this in commit 9394bf43b0 on Mar 14, 2019

PastaPastaPasta referenced this in commit c59929ee41 on Mar 15, 2019

PastaPastaPasta referenced this in commit 782092cc81 on Mar 16, 2019

PastaPastaPasta referenced this in commit ac857f6c53 on Apr 3, 2019

PastaPastaPasta referenced this in commit ee22cdcaac on Apr 3, 2019

PastaPastaPasta referenced this in commit 54ca7ea986 on Apr 5, 2019

PastaPastaPasta referenced this in commit dd91bd7b3a on May 6, 2019

barrystyle referenced this in commit 345c67b1b5 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

Wshadow: various gcc fixes #9911