8000 refactor: make member functions const when applicable by aureleoules · Pull Request #25673 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: make member functions const when applicable #25673

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
wants to merge 2 commits into from
Closed

refactor: make member functions const when applicable #25673

wants to merge 2 commits into from

Conversation

aureleoules
Copy link
Contributor
@aureleoules aureleoules commented Jul 22, 2022 •
edited
Loading

I have added a clang-tidy check: 'readability-make-member-function-const'.

Finds non-static member functions that can be made const because the functions don’t use this in a non-const way.

I believe this makes the code more maintainable.

I used clang-tidy with --fix-errors and the check readability-make-member-function-const to generate the refactor commit.

jonatack
jonatack reviewed Jul 22, 2022
View reviewed changes
Copy link
Member
@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Codebase-wide changes like this tend to be a difficult sell. Some initial thoughts.

Pro:

Caveat:

  • Const member functions need to be thread safe

Related:

@maflcko
Copy link
Member
maflcko commented Jul 22, 2022

std::move will cast even mutable references, which makes hard-to-find bugs and is thus generally not wanted if the reference was passed to a function. So I don't think replacing forward with move is correct, unless you can provide a reason for the change.

@aureleoules
Copy link
Contributor Author

std::move will cast even mutable references, which makes hard-to-find bugs and is thus generally not wanted if the reference was passed to a function. So I don't think replacing forward with move is correct, unless you can provide a reason for the change.

my mistake, I've dropped the commit

@maflcko
Copy link
Member
maflcko commented Jul 22, 2022

Yeah, we should probably check this with https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html

@DrahtBot
Copy link
Contributor
DrahtBot commented Jul 23, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
  • #24313 (Improve display address handling for external signer by Sjors)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Jul 23, 2022
Merged
Closed
Merged
Closed
Closed
jonatack
jonatack reviewed Jul 23, 2022
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Jul 24, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Jul 27, 2022
Draft
@fanquake
Copy link
Member

I assume this is being done with some automated tooling? If so, could you expand the PR description to describe what you are doing to produce the change?

Similar to #25707, if you want to make a change like this, we'd likely also want this enforced/linted in some way. Are there any checks you can use in https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/list.html?

@jonatack
Copy link
Member

Can tooling ensure these member functions marked as const are all thread-safe?

@aureleoules aureleoules force-pushed the 2022-07-refactor branch 3 times, most recently from 053dd01 to f73c294 Compare July 27, 2022 19:30
This was referenced Jul 27, 2022
Draft
Merged
This was referenced Jul 27, 2022
Merged
Merged
Merged
@aureleoules
Copy link
Contributor Author

I assume this is being done with some automated tooling? If so, could you expand the PR description to describe what you are doing to produce the change?

Done.

Also rebased.

This was referenced Aug 4, 2022
Merged
Closed
This was referenced Aug 13, 2022
Closed
Merged
@aureleoules aureleoules force-pushed the 2022-07-refactor branch 2 times, most recently from fadb393 to cb9009b Compare August 22, 2022 08:21
@DrahtBot DrahtBot mentioned this pull request Sep 9, 2022
Merged
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@maflcko
Copy link
Member
maflcko commented Oct 13, 2022

why the close? Seems a risk free CI check to avoid spending time on this during review?

@aureleoules
Copy link
Contributor Author

@MarcoFalke I was asked during coredev to close this.

This kind of code review-heavy codebase-wide refactor is not appropriate in general, especially for a new contributor. Constness can be accidental, and code/logical constness are not the same thing.

@aureleoules aureleoules deleted the 2022-07-refactor branch November 2, 2022 10:51
@fanquake fanquake mentioned this pull request Dec 8, 2022
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonatack jonatack jonatack left review comments

Assignees
No one assigned
Labels
Needs rebase Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aureleoules @maflcko @DrahtBot @fanquake @jonatack
0