See comment here: #30161 (comment)
Our code failed these checks in three places, which have been fixed up here. Though these appear to have been harmless, adding the check avoids the copy in the self-assignment case so there should be no downside.
~Additionally, minisketch failed the check as well. See https://github.com/sipa/minisketch/pull/87~ Edit: Done
After fixing up the violations, turn on the aggressive clang-tidy check.
Note for reviewers: git diff -w makes this trivial to review.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
@maflcko Hmm, how best to ignore the tidy false-positive in a leveldb header?
./leveldb/include/leveldb/status.h:106:24: error: operator=() does not handle self-assignment properly
It looks like we could add ExcludeHeaderFilterRegex to our tidy config, but that’s not wired up until clang-tidy-19.
Hmm, how best to ignore the tidy false-positive in a leveldb header?
FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
Is that actually the behaviour we want for all subtrees? Sounds like it could just hide bugs, and no-one is running any checks in the upstream repos.
Hmm, how best to ignore the tidy false-positive in a leveldb header?
FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
I believe (based on our .bear-tidy-config) that this is the case in current master as well. That’s not the issue here, though.
What we’re running into is a leveldb header that’s included by dbwrapper.cpp, so excluding the leveldb build itself doesn’t help.
Both of these cases appear to be harmless, but adding the tests allows us to
turn on the aggressive clang-tidy checks.
