avoid shadowing vars, and enable -Wshadow #8105

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

Relavant bug: #8102 (comment)

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

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

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

Many of the nVersion variables are totally unnecessary.

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

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.

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)

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.

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

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!

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.

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

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

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

I haven't double-checked them all yet.

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.

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.

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

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

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

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.

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.

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

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.

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