This diff can be reproduced by running (tested with clang-14 and clang-15):
0./autogen.sh && CC=clang CXX=clang++ ./configure --enable-suppress-external-warnings --enable-debug
1bear --config src/.bear-tidy-config -- make -j $(nproc)
2cd ./src/ && run-clang-tidy -quiet -fix -j $(nproc)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | hebasto |
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.
Besides, I’m also not sold that this lint is an actual improvement.
I think this PR diff is an improvement.
I think this PR diff is an improvement.
I’ll undraft this then, even if I still don’t understand why clang-tidy seems to catch more with debug enabled.
Besides, I’m also not sold that this lint is an actual improvement.
I think this PR diff is an improvement.
Could you explain why it’s an improvement? It’s unclear to me why const references to iterators would be an improvement, I was under the impression that iters by value were fine.
I’ll undraft this then, even if I still don’t understand why
clang-tidyseems to catch more with debug enabled.
I suspect The culprit is the -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE definition.
UPD. The mentioned defined macro makes sizeof(CTxMemPool::txiter) == 40 on my Ubuntu 22.04 x86_64, which makes clang-tidy consider this type expensive to copy.
Besides, I’m also not sold that this lint is an actual improvement.
I think this PR diff is an improvement.
Could you explain why it’s an improvement? It’s unclear to me why const references to iterators would be an improvement, I was under the impression that iters by value were fine.
Well, my main point was enforcing of const-corectness, but I missed the fact that CTxMemPool::txiter is a const_iterator.
Making a constness at the call site obvious for a code reader/reviewer is a very minor improvement, which could be just ignored.
Making a constness at the call site obvious for a code reader/reviewer is a very minor improvement, which could be just ignored.
Ok, so it seems we agree there is no code quality benefit. Is the motivation = CI already failing or wanting to add –enable-debug to it?
The failure here is #27316.
I’m having trouble understanding the relationship between this PR and that issue, am I missing something?
I’m having trouble understanding the relationship between this PR and that issue, am I missing something?
The CI was previously failing on the functional tests due to that issue.
The CI was previously failing on the functional tests due to that issue.
How is it possible for that functional test to fail due to using iters vs references to iters? Wasn’t the issue with #27316 not waiting for sync, i.e. addressed by #27318? Does the problem still exist? Still trying to understand the motivation for this PR.
Ok, so it seems we agree there is no code quality benefit.
To clarify my point, this change does improve code performance when the build is configured with --enable-debug.
I’m not saying that this fact justifies this PR sufficiently, but it should be considered, at the very least.
Is the motivation = CI already failing or wanting to add –enable-debug to it?
I’d avoid it as it will change code paths being parsed now (for example, the DEBUG_LOCKCONTENTION macro). Maybe just provide CPPFLAGS=-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE?
This diff can be reproduced by running:
```
./autogen.sh && CC=clang CXX=clang++ CPPFLAGS=-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE ./configure --enable-suppress-external-warnings --enable-debug
bear --config src/.bear-tidy-config -- make -j $(nproc)
cd ./src/ && run-clang-tidy -quiet -fix -j $(nproc)
```
Updated 73afe2c3ac87049887e0633bd25c20f809e8090c -> efa1d5bb477dc2093bc06ffded382fdce6bc7703 (clangTidyDebug_0 -> clangTidyDebug_1, compare).
Rebased efa1d5bb477dc2093bc06ffded382fdce6bc7703 -> 3e76aff9d4709f44d6439cd0cbc2fd6c90cae6ab (clangTidyDebug_1 -> clangTidyDebug_2, compare).
Maybe just provide CPPFLAGS=-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE?
Concept ~0 on making our clang-tidy usage dependant on some Boost define.
ACK 3e76aff9d4709f44d6439cd0cbc2fd6c90cae6ab
Concept ~0 on making our clang-tidy usage dependant on some Boost define.
What are possible drawbacks here?
In this case the -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE is just a subset of flags being used for debug builds.
What are possible drawbacks here?
I don’t really understand adding it given your comment above:
I’d avoid it as it will change code paths being parsed now
You suggested avoiding changing the code paths being parsed, and then suggested adding a define that changes the code paths being parsed?
In its current state, this PR is adding a Boost define, because it has a side-effect that basically “tricks” clang-tidy into thinking some code is slightly less performant (but only in debug builds) than it actually is, and thus will “lint” this code, and tell us to add more code to keep it quiet.
We could instead probably just add the consts, and leave it at that, without trying to invoke some boost-safe-mode-based side effect to trick a linter, and enforce some code change that in reality, doesn’t make any difference to anything.
Updated 3e76aff9d4709f44d6439cd0cbc2fd6c90cae6ab -> 27d182cef72e086afc4592cdfc565b3fdb52d0b4 (clangTidyDebug_2 -> clangTidyDebug_3, compare).
re: #27353 (comment)
or I think we could just close this?
Having #27724 been merged, I agree to close this PR.