The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See 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.

Conflicts

No conflicts as of last run.

The failure here is #27316.

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-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.

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.

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.

The functional test failure was sporadic, and unrelated to the changes proposed here.

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?

Updated 73afe2c -> efa1d5b (clangTidyDebug_0 -> clangTidyDebug_1, compare).

• Addressed @hebasto 's suggestion for adding the flag to the CI tidy job.

Rebased efa1d5b -> 3e76aff (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.

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.

@TheCharlatan can you update this to drop 3e76aff, or I think we could just close this?

I'll drop the commit. Could this be related to #27586 and solve some of the performance issues?

Updated 3e76aff -> 27d182c (clangTidyDebug_2 -> clangTidyDebug_3, compare).

• Dropped the commit changing the CI.

re: #27353 (comment)

or I think we could just close this?

Having #27724 been merged, I agree to close this PR.