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.
Codebase-wide changes like this tend to be a difficult sell. Some initial thoughts.
Pro:
Caveat:
Related:
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.
std::movewill 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
Yeah, we should probably check this with https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
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 {
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)
Thanks, I was wondering what this meant. I removed these changes.
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?
Can tooling ensure these member functions marked as const are all thread-safe?
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.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
why the close? Seems a risk free CI check to avoid spending time on this during review?
@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.