-
Notifications
You must be signed in to change notification settings - Fork 38.6k
Enable -Wshadow by default #8794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
doc/developer-notes.md
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Concept ACK |
|
utACK. 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. |
|
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. |
|
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. |
|
The univalue upstream merge (#8807) brought the total number of warnings here down to 3197. |
|
Current master w/ clang 3.9.0, full build (including GUI). No extra PRs applied. |
|
Current master (after merging #8655) brought the number of Wshadow warnings in a full build (with clang) down to 3167 |
|
@laanwj Can you upload the build log after |
|
@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. |
|
Great - this means there are 42 unique instances of this warning. Getting closer to zero :-) |
|
Concept ACK. Optimistically tagging this for 0.14.0 |
|
@fanquake Thanks for testing, Michael! |
|
Since the two pulls got merged, anything holding this back besides maybe #8808? |
|
Yes, thanks for testing @fanquake. 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). |
|
Just retested with master + this PR. Can confirm that there are no -Wshadow warnings. |
1e70349 to
359bac7
Compare
|
Thanks everyone for testing! |
|
Going to revert this, even though I tested this with two compilers before merging, people on IRC are apperently noticing floods of Wshadow warnings. |
Test and enabled
-Wshadowby 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.]