Wshadow: various gcc fixes #9911

paveljanik · 2017-03-03T17:37:46Z

Additional -Wshadow fixes for gcc 4.8.5.

paveljanik · 2017-03-03T18:14:22Z

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].

gmaxwell

utACK (lightly reviewed)

maflcko · 2017-03-04T20:02:09Z

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 ***@***.***> 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 · 2017-03-04T20:20:01Z

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 · 2017-03-04T20:42:51Z

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 · 2017-03-04T21:32:31Z

Sigh, more Wshadow changes. Is this ever done?

paveljanik · 2017-03-04T21:59:05Z

... 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 ;-)

laanwj · 2017-03-05T10:21:36Z

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

utACK

paveljanik · 2017-03-06T16:24:34Z

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

For easier searching, here is the log:

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

gmaxwell · 2017-03-06T23:25:07Z

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 · 2017-03-07T05:49:34Z

@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 · 2017-03-07T17:23:51Z

utACK bfc6be9

TheBlueMatt · 2017-03-08T20:40:33Z

src/torcontrol.cpp

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

With gcc4.8.5:

bitcoin@linux-5bz0:~/bitcoin> make CC=gcc Making all in src make[1]: Entering directory '/home/bitcoin/bitcoin/src' make[2]: Entering directory '/home/bitcoin/bitcoin/src' CXX libbitcoin_server_a-torcontrol.o /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]': /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]' /usr/include/boost/date_time/posix_time/date_duration_operators.hpp:33:37: required from here torcontrol.cpp:665:20: warning: shadowed declaration is here [-Wshadow] struct event_base *base; ^ /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]': /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]' /usr/include/boost/date_time/posix_time/conversion.hpp:30:48: required from here torcontrol.cpp:665:20: warning: shadowed declaration is here [-Wshadow]

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.

Hmm. I do not see any base there 8)

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

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 · 2017-03-08T21:15:19Z

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.

maflcko · 2017-03-08T21:31:52Z

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 ***@***.***> 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 · 2017-03-08T21:43:50Z

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 · 2017-03-09T07:34:01Z

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

  -Wshadow-local which warns if a local variable shadows another local
  variable or parameter,

  -Wshadow-compatible-local which warns if a local variable shadows
  another local variable or parameter whose type is compatible with that
  of the shadowing variable.

paveljanik · 2017-03-09T07:38:41Z

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 · 2017-03-09T07:41:45Z

Rebased.

paveljanik · 2017-03-09T08:22:11Z

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

paveljanik · 2017-03-15T14:43:45Z

Added fix for #9497's shadowing issues:

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

laanwj · 2017-03-18T09:16:41Z

@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.

d7f80b6 Rename first iterator to prevent shadowing. (Pavel Janík) b42ff60 Fix shadowing of local variables. (Pavel Janík) c4b60b3 Make some global variables less-global (static) (Pavel Janík) bb2aaee Prevent -Wshadow warnings with gcc versions 4.8.5, 5.3.1 and 6.2.1. (Pavel Janík) Tree-SHA512: 3aea4e28146c8f2a31351c6e2b0cce88b6f1e567a0ea0e6131624453e7193d0904e30d81b1439d8c69e281cf0e369b895851fb882ae48d5967b5c2e2c227404e

This warning was enabled by default in bitcoin#8808 but it's a [continuing](bitcoin#9911 (comment)) [source](bitcoin#10089 (comment)) of [annoyance](bitcoin#9911 (comment)) for me and other developers. I'm sick of sounding like a broken record, so disable it again.

d7f80b6 Rename first iterator to prevent shadowing. (Pavel Janík) b42ff60 Fix shadowing of local variables. (Pavel Janík) c4b60b3 Make some global variables less-global (static) (Pavel Janík) bb2aaee Prevent -Wshadow warnings with gcc versions 4.8.5, 5.3.1 and 6.2.1. (Pavel Janík) Tree-SHA512: 3aea4e28146c8f2a31351c6e2b0cce88b6f1e567a0ea0e6131624453e7193d0904e30d81b1439d8c69e281cf0e369b895851fb882ae48d5967b5c2e2c227404e

paveljanik changed the title ~~WIP: Wshadow: gcc 4.8.5 fixes~~ WIP: Wshadow: various gcc fixes Mar 3, 2017

fanquake added the Refactoring label Mar 3, 2017

gmaxwell reviewed Mar 4, 2017

View reviewed changes

paveljanik force-pushed the 20170303_Wshadow_streams branch from b43439c to 279c29d Compare March 4, 2017 21:57

paveljanik changed the title ~~WIP: Wshadow: various gcc fixes~~ Wshadow: various gcc fixes Mar 4, 2017

dcousens approved these changes Mar 5, 2017

View reviewed changes

maflcko mentioned this pull request Mar 6, 2017

Rename argument name 'data' to prevent shadowing. #9926

Closed

paveljanik force-pushed the 20170303_Wshadow_streams branch from 279c29d to bfc6be9 Compare March 6, 2017 16:23

TheBlueMatt reviewed Mar 8, 2017

View reviewed changes

paveljanik force-pushed the 20170303_Wshadow_streams branch from 5396531 to 8de5159 Compare March 9, 2017 07:41

paveljanik force-pushed the 20170303_Wshadow_streams branch from 8de5159 to 0cebfcf Compare March 15, 2017 14:40

Rename first iterator to prevent shadowing.

d7f80b6

paveljanik force-pushed the 20170303_Wshadow_streams branch from 83fedab to d7f80b6 Compare March 18, 2017 07:00

laanwj merged commit d7f80b6 into bitcoin:master Mar 18, 2017

str4d mentioned this pull request Mar 24, 2017

Tor ephemeral hidden services zcash/zcash#2177

Merged

laanwj mentioned this pull request Mar 27, 2017

Fix shadowing of 'what' as described in #10080. #10089

Closed

laanwj mentioned this pull request Apr 1, 2017

build: Disable Wshadow warning #10136

Merged

laanwj mentioned this pull request Feb 10, 2019

build: Disallow shadowing variables via -isystem, -Wshadow #15377

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Wshadow: various gcc fixes #9911

Wshadow: various gcc fixes #9911

Uh oh!

Conversation

paveljanik commented Mar 3, 2017

Uh oh!

paveljanik commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 4, 2017 via email

Uh oh!

paveljanik commented Mar 4, 2017

Uh oh!

paveljanik commented Mar 4, 2017

Uh oh!

laanwj commented Mar 4, 2017

Uh oh!

paveljanik commented Mar 4, 2017

Uh oh!

laanwj commented Mar 5, 2017

Uh oh!

dcousens left a comment

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Mar 6, 2017

Uh oh!

paveljanik commented Mar 7, 2017

Uh oh!

jtimon commented Mar 7, 2017

Uh oh!

TheBlueMatt Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 8, 2017

Uh oh!

maflcko commented Mar 8, 2017 via email

Uh oh!

laanwj commented Mar 8, 2017

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

paveljanik commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented Mar 9, 2017

Uh oh!

paveljanik commented Mar 9, 2017

Uh oh!

paveljanik commented Mar 15, 2017

Uh oh!

laanwj commented Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

paveljanik commented Mar 3, 2017 •

edited

Loading

gmaxwell left a comment •

edited

Loading

paveljanik commented Mar 6, 2017 •

edited

Loading

paveljanik commented Mar 9, 2017 •

edited

Loading

laanwj commented Mar 18, 2017 •

edited

Loading