fanquake requested review from glozow
on Mar 31, 2023
fanquake requested review from hebasto
on Mar 31, 2023
hebasto
commented at 12:55 pm on March 31, 2023:
member
Besides, I’m also not sold that this lint is an actual improvement.
I think this PR diff is an improvement.
TheCharlatan marked this as ready for review
on Mar 31, 2023
TheCharlatan
commented at 1:18 pm on March 31, 2023:
contributor
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.
glozow
commented at 1:19 pm on March 31, 2023:
member
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.
hebasto
commented at 2:43 pm on March 31, 2023:
member
I’ll undraft this then, even if I still don’t understand why clang-tidy seems 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.
hebasto
commented at 2:48 pm on March 31, 2023:
member
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::txiteris 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.
glozow
commented at 8:46 am on April 3, 2023:
member
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?
I’m having trouble understanding the relationship between this PR and that issue, am I missing something?
TheCharlatan
commented at 8:55 am on April 3, 2023:
contributor
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.
glozow
commented at 9:12 am on April 3, 2023:
member
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.
fanquake
commented at 9:13 am on April 3, 2023:
member
The functional test failure was sporadic, and unrelated to the changes proposed here.
hebasto
commented at 12:51 pm on April 3, 2023:
member
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?
TheCharlatan force-pushed
on Apr 18, 2023
refactor (tidy): Fixes after enable-debug configure
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)
```
27d182cef7
TheCharlatan force-pushed
on Apr 18, 2023
TheCharlatan
commented at 3:17 pm on April 18, 2023:
contributor
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.
fanquake
commented at 3:05 pm on May 8, 2023:
member
@TheCharlatan can you update this to drop 3e76aff9d4709f44d6439cd0cbc2fd6c90cae6ab, or I think we could just close this?
TheCharlatan
commented at 3:14 pm on May 8, 2023:
contributor
I’ll drop the commit. Could this be related to #27586 and solve some of the performance issues?
TheCharlatan force-pushed
on May 10, 2023
TheCharlatan
commented at 6:25 am on May 10, 2023:
contributor
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-10-30 03:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me