Enable clang-tidy checks for self-assignment #30234
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
Concept ACK. I've created a similar branch this morning following the discussion in #30161 :) |
theuni
force-pushed
the
clang-tidy-self-assign
branch
from
466e757 to
428932f
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being 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?
It looks like we could add |
|
Could add a patch to the leveldb subtree? |
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. |
I believe (based on our What we're running into is a leveldb header that's included by |
Belt-and suspenders after bitcoin#30234. Self-assignment should be safe _and_ discouraged. We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.
|
Added a commit to work around in leveldb. @fanquake Would you be ok with carrying this in our subtree? I'm not sure of a better solution. |
15796d4 build: warn on self-assignment (Cory Fields) 53372f2 refactor: disable self-assign warning for tests (Cory Fields) Pull request description: Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged. We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore. ACKs for top commit: maflcko: ACK 15796d4 fanquake: ACK 15796d4 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case. Tree-SHA512: 1f95f7c730b974ad1da55ebd381040bac312f2f380fff9d569ebab91d7c1963592a84d1613d81d96238c6f5a66aa40deebba68a76f6b24b02150d0a77c769654
Yea I think that's fine. |
…f97a96d9eb676c cb59af3 Squashed 'src/minisketch/' changes from 3472e2f5ec..eb37a9b8e7 (fanquake) Pull request description: Includes bitcoin-core/minisketch#87 which is used in #30234. Includes bitcoin-core/minisketch#88 which is used in #29876. ACKs for top commit: sipa: utACK 89464ad theuni: utACK 89464ad Tree-SHA512: 838a8c60856bfdf714da7d5d97e31d458290849ba5007d5c5bb7abb83d413ada6b4c16e45b0e060ff892b5785e6b664be9b6a666d04f0a414b0e359d64d3ad44
7045a90 Ignore clang's self-assignment check (Cory Fields) Pull request description: As-documented in the code, this is already safe so ignore the false-positive. Necessary to turn on the option in bitcoin/bitcoin#30234. Passes tests there. ACKs for top commit: maflcko: ACK 7045a90 Tree-SHA512: 7afa7d4e170e84f022fde037f4de6e417ad08bbc0e3953f1f3ea88da41cda1cc913a70ce43d4f6973813e78b0a0a97238deb44f79b6089688094b1e27ed69e51
theuni
force-pushed
the
clang-tidy-self-assign
branch
from
7b6dd5d to
2da83de
Compare
a37778d Squashed 'src/leveldb/' changes from e2f10b4e47..688561cba8 (fanquake) Pull request description: Includes bitcoin-core/leveldb-subtree#41 which is used in #30234. ACKs for top commit: theuni: utACK 95812d9 Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
…idy check Both of these cases appear to be harmless, but adding the tests allows us to turn on the aggressive clang-tidy checks.
theuni
force-pushed
the
clang-tidy-self-assign
branch
from
2da83de to
26a7f70
Compare
a37778d Squashed 'src/leveldb/' changes from e2f10b4e47..688561cba8 (fanquake) Pull request description: Includes bitcoin-core/leveldb-subtree#41 which is used in bitcoin#30234. ACKs for top commit: theuni: utACK 95812d9 Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
a37778d Squashed 'src/leveldb/' changes from e2f10b4e47..688561cba8 (fanquake) Pull request description: Includes bitcoin-core/leveldb-subtree#41 which is used in bitcoin#30234. ACKs for top commit: theuni: utACK 95812d9 Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
15796d4 build: warn on self-assignment (Cory Fields) 53372f2 refactor: disable self-assign warning for tests (Cory Fields) Pull request description: Belt-and suspenders after bitcoin#30234. Self-assignment should be safe _and_ discouraged. We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore. ACKs for top commit: maflcko: ACK 15796d4 fanquake: ACK 15796d4 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case. Tree-SHA512: 1f95f7c730b974ad1da55ebd381040bac312f2f380fff9d569ebab91d7c1963592a84d1613d81d96238c6f5a66aa40deebba68a76f6b24b02150d0a77c769654
6 participants
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 bitcoin-core/minisketch#87Edit: Done
After fixing up the violations, turn on the aggressive clang-tidy check.
Note for reviewers:
git diff -wmakes this trivial to review.