Skip to content

Conversation

aureleoules
Copy link
Contributor

@aureleoules aureleoules commented Jul 22, 2022

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.

Copy link
Member

@jonatack jonatack left a comment

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 23, 2022

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.

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

@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
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants