refactor: make member functions const when applicable

aureleoules commented at 10:38 am on July 22, 2022: member

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.

fanquake added the label Refactoring on Jul 22, 2022

jonatack commented at 11:00 am on July 22, 2022: contributor

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

Pro:

By default make member functions const

Caveat:

Const member functions need to be thread safe

Don’t make data members const or references

maflcko commented at 11:05 am on July 22, 2022: member

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 force-pushed on Jul 22, 2022

aureleoules commented at 11:56 am on July 22, 2022: member

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 commented at 11:58 am on July 22, 2022: member

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

DrahtBot commented at 1:18 am on July 23, 2022: contributor

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.

in src/leveldb/util/env_posix.cc:338 in fe9f78aa21 outdated

334@@ -335,7 +335,7 @@ class PosixWritableFile final : public WritableFile {
335     return status;
336   }
337 
338-  Status WriteUnbuffered(const char* data, size_t size) {
339+  Status WriteUnbuffered(const char* data, size_t size) const {

jonatack commented at 9:47 am on July 23, 2022:

You are touching files in https://github.com/bitcoin-core/leveldb-subtree; changes to it need to be made there or upstream and not in bitcoin/bitcoin. See the CI linter failure: “FAIL: subtree directory was touched without subtree merge” (test/lint/git-subtree-check.sh)

aureleoules commented at 4:34 pm on July 24, 2022:

Thanks, I was wondering what this meant. I removed these changes.

aureleoules force-pushed on Jul 24, 2022

DrahtBot added the label Needs rebase on Jul 27, 2022

fanquake commented at 12:36 pm on July 27, 2022: 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 commented at 12:39 pm on July 27, 2022: contributor

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

aureleoules force-pushed on Jul 27, 2022

DrahtBot removed the label Needs rebase on Jul 27, 2022

aureleoules force-pushed on Jul 27, 2022

DrahtBot added the label Needs rebase on Aug 1, 2022

aureleoules force-pushed on Aug 1, 2022

aureleoules commented at 1:32 pm on August 1, 2022: 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?

Done.

Also rebased.

DrahtBot removed the label Needs rebase on Aug 1, 2022

aureleoules force-pushed on Aug 2, 2022

aureleoules force-pushed on Aug 8, 2022

DrahtBot added the label Needs rebase on Aug 19, 2022

aureleoules force-pushed on Aug 22, 2022

DrahtBot removed the label Needs rebase on Aug 22, 2022

DrahtBot added the label Needs rebase on Aug 30, 2022

refactor: make member functions const if applicable 057558626b

tidy: Add readability-make-member-function-const 9112ec7963

aureleoules force-pushed on Sep 5, 2022

DrahtBot removed the label Needs rebase on Sep 5, 2022

DrahtBot commented at 8:37 am on September 12, 2022: 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”.

DrahtBot added the label Needs rebase on Sep 12, 2022

aureleoules closed this on Oct 12, 2022

maflcko commented at 8:12 am on October 13, 2022: member

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

aureleoules commented at 8:58 am on October 13, 2022: member

@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 deleted the branch on Nov 2, 2022

bitcoin locked this on Nov 2, 2023

refactor: make member functions const when applicable #25673

Conflicts