Skip to content

refactor, wallet: Avoid variable shadowing #27087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
hebasto wants to merge 1 commit into from
Closed

refactor, wallet: Avoid variable shadowing #27087

hebasto wants to merge 1 commit into from

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Feb 12, 2023 •
edited
Loading

This PR is a follow up of #26661 that introduced a local variable shadowing which I've noticed while reviewing #25665.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 12, 2023 •
edited
Loading

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.

@hebasto hebasto mentioned this pull request Feb 12, 2023
Merged
@hebasto
Copy link
Member Author

hebasto commented Feb 12, 2023

cc @furszy

furszy
furszy reviewed Feb 13, 2023
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This was referenced Feb 13, 2023
Merged
Merged
Draft
Open
@hebasto hebasto changed the title refactor: Rename variable to avoid shadowing refactor, wallet: Avoid variable shadowing Feb 13, 2023
@hebasto
Copy link
Member Author

hebasto commented Feb 13, 2023

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

Thanks! Done.

stickies-v
stickies-v approved these changes Feb 13, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3fb01a9

furszy
furszy approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3fb01a9

@fanquake
Copy link
Member

fanquake commented Feb 13, 2023

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
Copy link
Member Author

hebasto commented Feb 13, 2023

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 hebasto closed this Feb 13, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@furszy furszy furszy approved these changes

@kbakdev kbakdev kbakdev approved these changes

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hebasto @DrahtBot @fanquake @furszy @kbakdev @stickies-v