refactor, wallet: Avoid variable shadowing #27087

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:230212-shadow changing 1 files +1 −3

hebasto commented at 4:55 PM on February 12, 2023: member

This PR is a follow up of #26661 that introduced a local variable shadowing which I've noticed while reviewing #25665.
DrahtBot commented at 4:55 PM on February 12, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK stickies-v, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
DrahtBot added the label Refactoring on Feb 12, 2023
hebasto cross-referenced this on Feb 12, 2023 from issue wallet: Coin Selection, return accurate error messages by furszy
hebasto commented at 4:58 PM on February 12, 2023: member

cc @furszy
kbakdev approved
refactor: wallet, avoid shadowing, remove unneeded res variable. 3fb01a94aa
furszy commented at 12:33 AM on February 13, 2023: member

Could remove the top variable instead of renaming the internal one: https://github.com/furszy/bitcoin-core/commit/3fb01a94aa29dec9c546c0d0e9e54a513b3dee43.
DrahtBot cross-referenced this on Feb 13, 2023 from issue refactor: Replace `std::optional<bilingual_str>` with `util::Result` by ryanofsky
DrahtBot cross-referenced this on Feb 13, 2023 from issue wallet: group outputs only once, decouple it from Coin Selection by furszy
DrahtBot cross-referenced this on Feb 13, 2023 from issue refactor: Use util::Result class for wallet loading by ryanofsky
DrahtBot cross-referenced this on Feb 13, 2023 from issue refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky
hebasto force-pushed on Feb 13, 2023
hebasto renamed this:
~~refactor: Rename variable to avoid shadowing~~
refactor, wallet: Avoid variable shadowing
on Feb 13, 2023
hebasto commented at 9:44 AM on February 13, 2023: member

Could remove the top variable instead of renaming the internal one: furszy@3fb01a9.

Thanks! Done.
stickies-v approved
stickies-v commented at 12:05 PM on February 13, 2023: contributor

ACK 3fb01a94aa29dec9c546c0d0e9e54a513b3dee43
furszy approved
furszy commented at 12:40 PM on February 13, 2023: member

ACK 3fb01a9
fanquake commented at 1:36 PM on February 13, 2023: member

There are currently multiple instances of -Wshadow warnings emmited when compiling the codebase. Is there some reason this one specifically needs fixing? Otherwise this just introduces a pointless merge conflict to a number of very recently-rebased PRs.
hebasto commented at 2:38 PM on February 13, 2023: member

There are currently multiple instances of -Wshadow warnings emmited when compiling the codebase.

It was not about compiler warning, rather about code readability.

Is there some reason this one specifically needs fixing?

Probably subjective, but there was some extra mental burden while reviewing other PRs which touch this code.

Otherwise this just introduces a pointless merge conflict to a number of very recently-rebased PRs.

I hoped that just renaming (the first version of this PR) will not introduce any conflicts.
hebasto closed this on Feb 13, 2023
bitcoin locked this on Feb 13, 2024