Do not shadow member variables #8109

1. 
   paveljanik commented at 6:03 am on May 27, 2016: contributor

   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…

2. 
   jonasschnelli commented at 12:54 pm on May 27, 2016: contributor

   ConceptACK.

   • +1 for In to avoid shadowing.
3. 
   dcousens commented at 9:44 am on May 28, 2016: contributor

   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.
4. 
   sipa commented at 11:21 am on May 28, 2016: member

   Using varIn is more consistent with the rest of the codebase, I think, but no strong opinion either way.
5. 
   laanwj commented at 7:17 am on May 30, 2016: member

   Meh, doing x(x) where x is an argument as well as a member variable is valid c++.
6. 
   paveljanik commented at 8:36 am on May 30, 2016: contributor

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

7. 
   laanwj commented at 8:42 am on May 30, 2016: member

   @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.
8. 
   paveljanik commented at 8:51 am on May 30, 2016: contributor

   No problem, my fault. I’ll add a link to the first comment here.
9. 
   MarcoFalke commented at 4:04 pm on June 3, 2016: member

   utACK 06363466052bfac13849e65c76333e8d3a1fdf27
10. 
    paveljanik commented at 4:11 pm on June 3, 2016: contributor

    Thinking more about it, we should use the style used “around” the code…
11. 
    laanwj commented at 12:43 pm on June 7, 2016: member

    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)

12. 
    paveljanik commented at 1:17 pm on June 7, 2016: contributor

    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.

13. 
    laanwj commented at 1:44 pm on June 7, 2016: member

    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.

14. 
    paveljanik commented at 1:59 pm on June 7, 2016: contributor

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

15. 
    laanwj commented at 2:07 pm on June 7, 2016: member

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

16. 
    paveljanik commented at 2:16 pm on June 7, 2016: contributor

    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…
17. 
    sipa commented at 2:36 pm on June 7, 2016: member

    BUGS!
18. paveljanik force-pushed on Jun 7, 2016
19. 
    paveljanik commented at 5:16 pm on June 7, 2016: contributor

    While investigating the groups of related changes, I have found one more related to HTTP interface thus I added it here.
20. paveljanik force-pushed on Jun 9, 2016
21. 
    laanwj commented at 8:32 am on June 9, 2016: member

    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.

22. 
    MarcoFalke commented at 11:37 am on June 9, 2016: member

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

    utACK c9cf1b9

23. 
    sipa commented at 1:19 pm on June 9, 2016: member

    utACK c9cf1b9de9346540cd8730e9a4111eaafb51ecd8
24. Do not shadow member variables ff8d279a78
25. paveljanik force-pushed on Jul 31, 2016
26. 
    paveljanik commented at 6:56 pm on July 31, 2016: contributor

    Rebased.
27. 
    MarcoFalke commented at 5:09 pm on August 18, 2016: member

    utACK ff8d279 (gives same binaries, so likely no BUGS)
28. sipa merged this on Aug 26, 2016
29. sipa closed this on Aug 26, 2016

    ---

30. sipa referenced this in commit 9a0ed08b40 on Aug 26, 2016
31. codablock referenced this in commit 9222ebd6e5 on Sep 19, 2017
32. codablock referenced this in commit b9b0cc0dbf on Jan 9, 2018
33. codablock referenced this in commit 30066a6004 on Jan 9, 2018
34. andvgal referenced this in commit a51db55717 on Jan 6, 2019
35. MarcoFalke locked this on Sep 8, 2021

Contributors
paveljanik jonasschnelli dcousens sipa laanwj MarcoFalke

Labels
Refactoring