Do not shadow member variables

[paveljanik]

I'm working on enabling -Wshadow in our builds (#8105).

This is a test. Do we prefer changes like this? Or different name changes (like req -> reqIn instead of _req)? Both styles are used in the source...

[jonasschnelli]

ConceptACK.

• +1 for In to avoid shadowing.

[dcousens]

Concept ACK for both _<var> and varIn, the latter is more explicit but seems like it has more potential for actual overlap with an existing variable name.

[sipa]

Using varIn is more consistent with the rest of the codebase, I think, but no strong opinion either way.

[laanwj]

Meh, doing x(x) where x is an argument as well as a member variable is valid c++.

[paveljanik]

@laanwj Sure, but...? It is valid C++ but can hide problems.

But I do not have a problem when we say we do not need/want -Wshadow...

[laanwj]

@paveljanik Yes, sorry, with the context (#8105) this makes sense. You should mention that. Out-of-context this seemed like just a bit of playing with coding style.

[paveljanik]

No problem, my fault. I'll add a link to the first comment here.

[maflcko]

utACK 06363466052bfac13849e65c76333e8d3a1fdf27

[paveljanik]

Thinking more about it, we should use the style used "around" the code...

[laanwj]

I slightly prefer _var.

Is this enough to allow enabling the -fshadow warnings afterwards? Or are there remaining cases of shadowing? I'd like to do this in one pull/project as much as possible, to avoid intermediate pulls introducing new problems here.
(as this is a trivial change, and we want to do this, let's just get it over with)

[paveljanik]

We use shadowing in many files.

You can see a preview/WIP here:
https://github.com/bitcoin/bitcoin/compare/master...paveljanik:20160526_Wshadow?expand=1

This is not doable (or at least reviewable) in one PR IMO.

[laanwj]

Whoa. That is kind of scary much, looks like a lot more work than I estimated it as.

To be honest I don't know how to handle a code change on that scale sanely.

But as the changes are all similar, I do still think it's less overhead to review in one go, than having 100 pulls that change this for one class at a time.

[paveljanik]

I can group pieces together in say 5 PRs. But still, this is not completely solved yet or some changes are too intrusive. And this is not needed to fix immediately. Thus I'd prefer separate PRs (with the help from others/part maintainers).

And yes, I too don't know how to handle a code change on that scale sanely. But what I know exactly is I do not want to do that in one PR ;-)

[laanwj]

For something like this it'd be ideal to be able to do it mechanically and deterministically.
Then people could review the script making the change and replicate the changes.
My clang-fu doesn't go that far though.

Otherwise, I agree, the best possibly is probably separate commits for separate parts of the code (GUI, RPC, etc).

[paveljanik]

This can't be done automatically, because there could be BUGS in the code, initialize shadowing, shadowing local variables etc. In some cases, the fix can be removed line...

[sipa]

BUGS!

[paveljanik]

While investigating the groups of related changes, I have found one more related to HTTP interface thus I added it here.

[laanwj]

because there could be BUGS in the code

Sure, though I think fixing BUGS should be separate from the pure refactoring step, which may make these BUGS apparent.

[maflcko]

c9cf1b9 gives the same binaries as c9cf1b9~1, so I am assuming there are no BUGS added nor any BUGS removed.

utACK c9cf1b9

[sipa]

utACK c9cf1b9de9346540cd8730e9a4111eaafb51ecd8

[paveljanik]

Rebased.

[maflcko]

utACK ff8d279 (gives same binaries, so likely no BUGS)