Skip to content

Conversation

@paveljanik
Copy link
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.]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the squashme commit.

@maflcko
Copy link
Member

maflcko commented Sep 22, 2016

Concept ACK

@laanwj
Copy link
Member

laanwj commented Sep 23, 2016

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
Copy link
Contributor Author

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
Copy link
Member

laanwj commented Sep 23, 2016

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
Copy link
Member

laanwj commented Sep 25, 2016

The univalue upstream merge (#8807) brought the total number of warnings here down to 3197.

@paveljanik
Copy link
Contributor Author

@laanwj "here" means with gcc, other PRs (see the end of #8105) included? I'm now targeting gcc to be shadow free...

@laanwj
Copy link
Member

laanwj commented Sep 25, 2016

Current master w/ clang 3.9.0, full build (including GUI). No extra PRs applied.

@laanwj
Copy link
Member

laanwj commented Sep 27, 2016

Current master (after merging #8655) brought the number of Wshadow warnings in a full build (with clang) down to 3167

@paveljanik
Copy link
Contributor Author

paveljanik commented Sep 27, 2016

@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
Copy link
Member

laanwj commented Sep 27, 2016

@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
Copy link
Contributor Author

Great - this means there are 42 unique instances of this warning. Getting closer to zero :-)

@fanquake
Copy link
Member

fanquake commented Nov 6, 2016

Concept ACK. Optimistically tagging this for 0.14.0

@fanquake fanquake added this to the 0.14.0 milestone Nov 6, 2016
@fanquake
Copy link
Member

fanquake commented Nov 7, 2016

With master + #9039 I only see one -Wshadow warning on OS X, which is fixed by #8981.

@paveljanik
Copy link
Contributor Author

@fanquake Thanks for testing, Michael!

@maflcko
Copy link
Member

maflcko commented Nov 9, 2016

Since the two pulls got merged, anything holding this back besides maybe #8808?

@laanwj
Copy link
Member

laanwj commented Nov 9, 2016

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
Copy link
Member

fanquake commented Nov 9, 2016

Just retested with master + this PR. Can confirm that there are no -Wshadow warnings.

@laanwj laanwj force-pushed the 20160922_Wshadow_enable branch from 1e70349 to 359bac7 Compare November 9, 2016 13:11
@laanwj laanwj merged commit 359bac7 into bitcoin:master Nov 9, 2016
laanwj added a commit that referenced this pull request Nov 9, 2016
359bac7 Add notes about variable names and shadowing (Pavel Janík)
fd5654c Check and enable -Wshadow by default. (Pavel Janík)
@paveljanik
Copy link
Contributor Author

Thanks everyone for testing!

@laanwj
Copy link
Member

laanwj commented Nov 9, 2016

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 pushed a commit to codablock/dash that referenced this pull request Jan 15, 2018
359bac7 Add notes about variable names and shadowing (Pavel Janík)
fd5654c Check and enable -Wshadow by default. (Pavel Janík)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
359bac7 Add notes about variable names and shadowing (Pavel Janík)
fd5654c Check and enable -Wshadow by default. (Pavel Janík)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 24, 2019
359bac7 Add notes about variable names and shadowing (Pavel Janík)
fd5654c Check and enable -Wshadow by default. (Pavel Janík)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants