Enable -Wshadow by default #8794

pull paveljanik wants to merge 2 commits into bitcoin:master from paveljanik:20160922_Wshadow_enable changing 2 files +27 −0

paveljanik commented at 2:42 PM on September 22, 2016: contributor

Test and enabled -Wshadow by default, add developer note about this change (see #8105).

As my native language is not English, I'm open to suggestions.

[Edits from maintainers allowed for easier cooperation.]
Check and enable -Wshadow by default. fd5654cab1

in doc/developer-notes.md:None in d5419fdd1a outdated

 330 | @@ -331,6 +331,31 @@ Strings and formatting
 331 |  
 332 |    - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion
 333 |  
 334 | +Variable names
 335 | +--------------
 336 | +
 337 | +The shadowing warning (`-Wshadow`) is enabled by default. Please name

laanwj commented at 2:48 PM on September 22, 2016:

Please mention the rationale why it is enabled, why we went through this in the first place: which is to avoid bugs like #8102

paveljanik commented at 5:09 PM on September 22, 2016:

Done in the squashme commit.

MarcoFalke commented at 10:38 PM on September 22, 2016: member

Concept ACK
dcousens approved
laanwj added the label Build system on Sep 23, 2016
laanwj commented at 11:50 AM on September 23, 2016: member
utACK. At this moment, though, this still generates 3406 warnings in the process of the build:
```
$ grep Wshadow /tmp/log|wc
   3406   30643  458032
```
I don't think this number needs to be 0 before merging this, but it does needs to be a manageable number to avoid 'shadowing' other warnings and errors, as well as increasing the Travis output by a thousandfold.
paveljanik commented at 11:54 AM on September 23, 2016: contributor

Definitely. This has to wait.

There are other PRs open, serialization changes has to be merged, and I still have to PR txmempool, main, wallet, torcontroller and tests changes.
laanwj commented at 11:59 AM on September 23, 2016: member

I'd say most urgent is to remove shadowing in (inline code in) header files, especially the often-included ones. Those warnings come back for every compiled file.
laanwj commented at 2:38 PM on September 25, 2016: member

The univalue upstream merge (#8807) brought the total number of warnings here down to 3197.
paveljanik commented at 2:44 PM on September 25, 2016: contributor

@laanwj "here" means with gcc, other PRs (see the end of #8105) included? I'm now targeting gcc to be shadow free...
laanwj commented at 2:47 PM on September 25, 2016: member

Current master w/ clang 3.9.0, full build (including GUI). No extra PRs applied.
laanwj commented at 1:48 PM on September 27, 2016: member

Current master (after merging #8655) brought the number of Wshadow warnings in a full build (with clang) down to 3167
paveljanik commented at 1:55 PM on September 27, 2016: contributor

@laanwj Can you upload the build log after grep "warning: declaration shadows" somewhere for me? Or at least present something like ... | sort | uniq -c | sort -rn | head ;-)
laanwj commented at 2:30 PM on September 27, 2016: member

@paveljanik Sure: https://dev.visucore.com/bitcoin/tmp/log.gz

I think what is left is nearly all serialization related, certainly the ones that appear many times.
paveljanik commented at 3:58 PM on September 27, 2016: contributor

Great - this means there are 42 unique instances of this warning. Getting closer to zero :-)
laanwj assigned theuni on Sep 29, 2016
fanquake commented at 12:40 PM on November 6, 2016: member

Concept ACK. Optimistically tagging this for 0.14.0
fanquake added this to the milestone 0.14.0 on Nov 6, 2016
fanquake commented at 8:49 AM on November 7, 2016: member

With master + #9039 I only see one -Wshadow warning on OS X, which is fixed by #8981.
paveljanik commented at 9:00 AM on November 7, 2016: contributor

@fanquake Thanks for testing, Michael!
MarcoFalke commented at 12:57 PM on November 9, 2016: member

Since the two pulls got merged, anything holding this back besides maybe #8808?
laanwj commented at 12:58 PM on November 9, 2016: member

Yes, thanks for testing @fanquake. I can confirm the same with LLVM 4.0 (git master, empty ccache) on Linux.

As #9039 is merged this is ready for merge after the squashme is squashed (done).

#8808 and the shadow warning in leveldb doesn't need to be a blocker, the reason for holding this off was the warning spam for every single file and that's gone now.

Will also try this with gcc just to be sure (done).
fanquake commented at 1:04 PM on November 9, 2016: member

Just retested with master + this PR. Can confirm that there are no -Wshadow warnings.
Add notes about variable names and shadowing 359bac7cff
laanwj force-pushed on Nov 9, 2016
laanwj merged this on Nov 9, 2016
laanwj closed this on Nov 9, 2016
laanwj referenced this in commit e0477f6d20 on Nov 9, 2016
paveljanik commented at 1:47 PM on November 9, 2016: contributor

Thanks everyone for testing!
laanwj commented at 8:27 PM on November 9, 2016: member

Going to revert this, even though I tested this with two compilers before merging, people on IRC are apperently noticing floods of Wshadow warnings.
codablock referenced this in commit 5917290514 on Jan 15, 2018
andvgal referenced this in commit 15548b3eea on Jan 6, 2019
CryptoCentric referenced this in commit 0dbed115fc on Feb 24, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.14.0

Linked (view graph)

#8105 avoid shadowing vars, and enable -Wshadow #29485 build: Enable -Wshadow