avoid shadowing vars, and enable -Wshadow

MarcoFalke commented at 6:45 pm on May 26, 2016: member

I haven’t looked into a fix for this, but it should be easy to implement and hopefully uncontroversial.

TODO:

univalue shadowing fix (merged, synced to our tree )
serialize.h and nVersion, nType collisions (~~#8468~~ #9039 )
LOCK inside LOCKshadowing (using __COUNTER__ for LOCK macro) (#8472)
other misc source fixes
enabling -Wshadow (#8794)
document -Wshadow and add developer notes about it (#8794)
leveldb shadowing fix ? (https://github.com/google/leveldb/pull/378, but fixed by other changes there.)
GCC Set #8808

theuni commented at 6:54 pm on May 26, 2016: member

@MarcoFalke We’re guilty of this all over the place (serialization in particular), so while it would be simple, the actual change-set would probably be rather large.

I’m all for it though, this is a long-standing annoyance of mine.

paveljanik commented at 7:38 pm on May 26, 2016: contributor

I remember I tried it already and failed. These nVersion variables were nightmare ;-)

sipa commented at 7:39 pm on May 26, 2016: member

Many of the nVersion variables are totally unnecessary.

paveljanik commented at 7:44 pm on May 26, 2016: contributor

I’ll dig the work/redo the configure stuff.

MarcoFalke commented at 8:35 pm on May 26, 2016: member

Even though it may not be possible to fix all at once, it is good to keep this in mind for new code or whenever code is touched/moved due to other reasons.

paveljanik commented at 8:52 am on May 27, 2016: contributor

Global tasks to be solved:

leveldb shadowing fix (https://github.com/google/leveldb/pull/378, but fixed by other changes there.)
univalue shadowing fix (merged, synced to our tree )
serialize.h and nVersion, nType collisions (~~#8468~~ #9039 )
LOCK inside LOCKshadowing (using __COUNTER__ for LOCK macro) (#8472)
other misc source fixes
enabling -Wshadow (#8794)
document -Wshadow and add developer notes about it (#8794)

MarcoFalke commented at 9:09 am on May 27, 2016: member

It is good to see progress on this, but it may be worth to wait until segwit (and maybe p2p-refactor) is merged to avoid unnecessary code-conflicts.

paveljanik commented at 9:12 am on May 27, 2016: contributor

Agreed, I’ll wait with submitting “destructive” changes. Working on it in private tree now (https://github.com/paveljanik/bitcoin/tree/20160526_Wshadow).

paveljanik commented at 6:48 pm on August 5, 2016: contributor

FYI: I was able to fix serialization issues. I’ll wait for #8109, #8163, #8191 and #8449 merge to get clean tree. Please help ACK them. Thank you!

paveljanik commented at 12:33 pm on September 4, 2016: contributor

Current status:

3 PRs opened.

Remaining: complete QT (33 files, 200 changes, will be done later), ~20 other files outside of qt. Approx. 5 changes call for more review and will be PRed separately.

MarcoFalke commented at 9:31 am on September 9, 2016: member

Could you submit the changes required for qt and then enable the warning, so we don’t have new instances popping up accidentally?

paveljanik commented at 9:55 am on September 9, 2016: contributor

I do not have them in the ready-to-merge quality yet.

Enabling the -Wshadow now is a no go from me, because there are approx. 500 warnings in qt now…

paveljanik commented at 11:44 am on September 9, 2016: contributor

I pushed my changes in qt here: https://github.com/bitcoin/bitcoin/commit/f9d00d7e624a6760a099ca61caca25d7982844d1

I haven’t double-checked them all yet.

MarcoFalke commented at 7:52 am on September 21, 2016: member

I haven’t double-checked them all yet.

As long as the binaries match, there is not much to lose. Also, for qt it should be easier to get in.

laanwj commented at 5:06 am on September 22, 2016: member

Yes, Qt will be much easier to merge. If that’s the blocker to enabling the warnings by default, let’s just get it over with.

paveljanik commented at 3:09 pm on September 23, 2016: contributor

Current status: with #8468, #8655, #8658, #8794 and upstream-leveldb fix, there are no Wshadow warnings on OS X.

paveljanik commented at 2:42 pm on September 25, 2016: contributor

I’ll now try to target some gcc version, so we are ready to enable -Wshadow by default and people on Linux boxes are not flowed by warnings…

fanquake commented at 1:21 pm on November 9, 2016: member

We still have at-least #8808 to be merged before this can be closed. Have updated the original issue with remaining TODOs.

laanwj commented at 9:33 pm on November 9, 2016: member

I don’t think enabling WShadow by default is worth doing anymore.

This is very much dependent on the compiler, and some compilers may be overly strict. When googling around about Wshadow there are tons of complaints about it being too strict for some compilers, or even having false positives. Enabling this by default for all compilers will result in an unending source of complaints.

I thought this was something that could be done quickly. It was not my intention to merge “remove WShadow warnings” pulls for the large part of a year. The cure seems worse than the ailment in this particular instance.

MarcoFalke commented at 9:42 pm on November 9, 2016: member

Closing this then.

Though, I think if #8808 is the last step, we could give it a try once and revert when it turns out to create more noise than signal.

So lets move the final discussion/decision to #8808 and close this issue.

MarcoFalke closed this on Nov 9, 2016

paveljanik commented at 9:55 pm on November 9, 2016: contributor

It makes no sense to close this, sorry.

The enable change was merged prematurely as I already said. #8808 was missing and thus gcc users could see some warnings. And gcc6 needs more but small changeset. I’ll have it ready tomorrow (@gmaxwell already tested the change at http://tmp.janik.cz/q.diff).

MarcoFalke commented at 10:20 pm on November 9, 2016: member

I think it is fine if you just cherry-pick fd5654c into 8808, no need to open a separate pull or keep this issue open.

paveljanik commented at 5:32 pm on November 16, 2016: contributor

Just for the record: another gcc, another small update to univalue https://github.com/jgarzik/univalue/pull/34.

DrahtBot locked this on Sep 8, 2021

avoid shadowing vars, and enable -Wshadow #8105