ci: Update Clang in “tidy” job #31306

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:241117-tidy changing 7 files +17 βˆ’10
  1. hebasto commented at 4:25 pm on November 17, 2024: member

    This PR switches to the latest IWYU 0.23, which is compatible with Clang 19.

    New “bugprone-use-after-move” and “modernize-use-starts-ends-with” warnings that emerged have been addressed.

  2. hebasto added the label Refactoring on Nov 17, 2024
  3. hebasto added the label Tests on Nov 17, 2024
  4. DrahtBot commented at 4:25 pm on November 17, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31306.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK l0rinc

    If your review is incorrectly listed, please react with πŸ‘Ž to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31308 (ci, iwyu: Treat warnings as errors for specific targets by hebasto)
    • #31061 (refactor: Check translatable format strings at compile-time by maflcko)
    • #30997 (build: Switch to Qt 6 by hebasto)
    • #30975 (Add multiprocess binaries to release build by Sjors)
    • #30906 (refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests by l0rinc)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. hebasto force-pushed on Nov 17, 2024
  6. in src/.clang-tidy:22 in 4b833038de outdated
    18@@ -19,8 +19,10 @@ performance-*,
    19 -performance-avoid-endl,
    20 -performance-enum-size,
    21 -performance-inefficient-string-concatenation,
    22+-performance-inefficient-vector-operation,
    


    l0rinc commented at 9:04 pm on November 17, 2024:
    Given that #31305 fixes the issue, are we disabling it as a temporary measure?

    hebasto commented at 9:07 pm on November 17, 2024:

    Right.

    It depends on which PR is merged first :)

  7. in ci/test/00_setup_env_native_tidy.sh:9 in 4b833038de outdated
     5@@ -6,9 +6,9 @@
     6 
     7 export LC_ALL=C.UTF-8
     8 
     9-export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
    10+export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.10"
    


    maflcko commented at 8:39 am on November 18, 2024:

    Not sure about using releases which are basically EOL in less than 6 months after the release branch is created. This requires touching the release branches to bump those along.

    My preference would be to use APT_LLVM_V


    hebasto commented at 11:37 am on November 25, 2024:
    Thanks! Switched to use APT_LLVM_V.
  8. in src/test/result_tests.cpp:66 in 4b833038de outdated
    63+void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, const Args&... args)
    64 {
    65     ExpectResult(result, true, str);
    66     BOOST_CHECK_EQUAL(result.has_value(), true);
    67-    BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
    68+    BOOST_CHECK_EQUAL(result.value(), T{args...});
    


    maflcko commented at 8:44 am on November 18, 2024:
    are you sure this is correct? Not that it matters, but isn’t it theoretically pessimising in ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));?

    hebasto commented at 11:39 am on November 25, 2024:
    This change has been removed from this PR.
  9. in src/test/coins_tests.cpp:774 in 4532e1d509 outdated
    770@@ -771,11 +771,10 @@ static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount mo
    771 // while still verifying that the CoinsViewCache::AddCoin implementation
    772 // ignores base values.
    773 template <typename... Args>
    774-static void CheckAddCoin(Args&&... args)
    


    l0rinc commented at 10:27 am on November 18, 2024:
    This was already removed in the ACK-ed https://github.com/bitcoin/bitcoin/pull/30906/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33L774-L777, could you please either revert this line or do it in a separate PR, since I think removing it completely is a better fix.

    hebasto commented at 11:39 am on November 25, 2024:
    This change has been removed from this PR.

    l0rinc commented at 12:37 pm on November 25, 2024:
    Thank you!

    l0rinc commented at 3:56 pm on November 27, 2024:
    I’d like to revive my original concerns here: c4eb31b (#30906) removes this method completely, which I think is a better fix for it than the current workaround.
  10. in src/test/result_tests.cpp:62 in 4532e1d509 outdated
    58@@ -59,12 +59,11 @@ void ExpectResult(const util::Result<T>& result, bool success, const bilingual_s
    59 }
    60 
    61 template <typename T, typename... Args>
    62-void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args&&... args)
    63+void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, const Args&... args)
    


    l0rinc commented at 10:35 am on November 18, 2024:

    Looking at the actual usage, ExpectSuccess doesn’t need to be a variadic template function:

    0ExpectSuccess(IntFn(5, true), {}, 5);
    1ExpectSuccess(NoCopyFn(5, true), {}, 5);
    2ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));
    

    , we might as well simplify to something like:

    0template <typename T, typename U>
    1void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, U&& expected)
    2{
    3    ExpectResult(result, true, str);
    4    BOOST_CHECK_EQUAL(result.has_value(), true);
    5    BOOST_CHECK_EQUAL(result.value(), expected);
    6    BOOST_CHECK_EQUAL(&result.value(), &*result);
    7}
    

    hebasto commented at 11:40 am on November 25, 2024:
    This change has been removed from this PR.

    maflcko commented at 8:04 am on November 28, 2024:

    Looking at the actual usage, ExpectSuccess doesn’t need to be a variadic template function:

    This may or may not be true right now, but the function has been intentionally written the way it is now. This is a pretty standard and common boilerplate code to pass a variable number of args to a constructor.

    Changing it would be problematic, because it will need to be touched again in the future to be reverted, if there is a constructor with more than one arg.

    Also, your suggestion will fail to compile if the constructor accepts a single arg and is marked explicit.

  11. l0rinc changes_requested
  12. l0rinc commented at 10:37 am on November 18, 2024: contributor
    Concept ACK, but approach NACK, I think some of the examples can be fixed by removing the “feature” instead of just patching them.
  13. in src/.clang-tidy:25 in 4b833038de outdated
    18@@ -19,8 +19,10 @@ performance-*,
    19 -performance-avoid-endl,
    20 -performance-enum-size,
    21 -performance-inefficient-string-concatenation,
    22+-performance-inefficient-vector-operation,
    23 -performance-no-int-to-ptr,
    24 -performance-noexcept-move-constructor,
    25+-performance-unnecessary-copy-initialization,
    


    l0rinc commented at 6:39 pm on November 24, 2024:
    Similarly to #31305, I’ve pushed a PR fixing this instead of disabling the check: https://github.com/bitcoin/bitcoin/pull/31364
  14. hebasto force-pushed on Nov 25, 2024
  15. hebasto commented at 11:41 am on November 25, 2024: member

    @maflcko @l0rinc

    Thank you for your reviews. The PR has been updated per your feedback.

  16. l0rinc approved
  17. l0rinc commented at 12:47 pm on November 25, 2024: contributor

    ACK 3e039b7a03bea6e11e994aebe038ebd6ca80aeb8

    The latest change:

    • Reverted the CI_IMAGE_NAME_TAG and added APT_LLVM_V.
    • Reverted the CheckAddCoin and ExpectSuccess fixes to avoid collisions with other PRs.
    • Updated description to reflect the dependent fix PRs.
  18. in src/wallet/walletdb.cpp:1016 in 3c259019ff outdated
    1012@@ -1013,7 +1013,7 @@ static DBErrors LoadAddressBookRecords(CWallet* pwallet, DatabaseBatch& batch) E
    1013             // "1" or "p" for present (which was written prior to
    1014             // f5ba424cd44619d9b9be88b8593d69a7ba96db26).
    1015             pwallet->LoadAddressPreviouslySpent(dest);
    1016-        } else if (strKey.compare(0, 2, "rr") == 0) {
    1017+        } else if (strKey.compare(0, 2, "rr") == 0) {  // NOLINT(modernize-use-starts-ends-with)
    


    maflcko commented at 12:53 pm on November 25, 2024:
    Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

    l0rinc commented at 12:59 pm on November 25, 2024:

    I’m fine either way, but I guess this way it was easier to selectively revert a conflicting fix and leave only the suppression.

    But I’d like to understand the reason for the supressions: would the lint problem break CI without them? Since this will still invalidate ACKed PRs if we merge it like this.


    hebasto commented at 1:24 pm on November 25, 2024:

    Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

    The commits have been squashed.

    But I’d like to understand the reason for the supressions: would the lint problem break CI without them?

    Yes, it would.

  19. hebasto force-pushed on Nov 25, 2024
  20. l0rinc commented at 1:27 pm on November 25, 2024: contributor
    ACK f90fddfabd646c3f1d21de7bb004707a072e753e
  21. in src/test/result_tests.cpp:71 in f90fddfabd outdated
    62@@ -63,6 +63,7 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
    63 {
    64     ExpectResult(result, true, str);
    65     BOOST_CHECK_EQUAL(result.has_value(), true);
    66+    // NOLINTNEXTLINE(bugprone-use-after-move)
    


    maflcko commented at 1:31 pm on November 25, 2024:
    it would be good to understand why this is needed. If this is a bug, it would be good to fix it.

    hebasto commented at 1:34 pm on November 25, 2024:
    Right. I outlined this issue for follow-ups in the PR description.

    l0rinc commented at 1:37 pm on November 25, 2024:
    We can likely simplify the call here, see: #31306 (review)

    hebasto commented at 1:43 pm on November 25, 2024:

    Fixing all new “bugprone-use-after-move” warnings contradicts with other comments. On the other hand, fixing some of them is not consistent. That’s why I chose to silence them.

    NOLINT comments are easy searchable and effectively document the issues that have to be resolved.


    l0rinc commented at 2:00 pm on November 25, 2024:
    Unfortunately silencing will also invalidate the ACK-ed PR. But if we could instead add -bugprone-use-after-move to src/.clang-tidy this concern could be addressed separately.

    hebasto commented at 2:03 pm on November 25, 2024:
    It will expose the whole codebase to re-introducing related bugs, no?

    l0rinc commented at 2:08 pm on November 25, 2024:
    We could tackle it in a separate pr like we do with the performance-* checks, right?

    hebasto commented at 2:19 pm on November 25, 2024:
    We could, but performance-* checks are not about safety, whereas bugprone-* checks are.

    l0rinc commented at 2:21 pm on November 25, 2024:
    If you decide to fix them here, I’ll just rebase the related PR and ask for a re-review, it’s not a tragedy :)

    maflcko commented at 2:45 pm on November 25, 2024:

    I looked at this and this is an upstream boost bug, not an llvm or bitcoin-core bug.

    The reason is that the macro is a loop (not a do-while-0-loop):

     0#define BOOST_TEST_TOOL_IMPL( frwd_type, P, assertion_descr, TL, CT, ARGS )     \
     1do {                                                                            \
     2    BOOST_TEST_PASSPOINT();                                                     \
     3    ::boost::test_tools::tt_detail::                                            \
     4    BOOST_PP_IF( frwd_type, report_assertion, check_frwd ) (                    \
     5        BOOST_JOIN( BOOST_TEST_TOOL_PASS_PRED, frwd_type )( P, ARGS ),          \
     6        BOOST_TEST_LAZY_MSG( assertion_descr ),                                 \
     7        BOOST_TEST_L(__FILE__),                                                 \
     8        static_cast<std::size_t>(__LINE__),                                     \
     9        ::boost::test_tools::tt_detail::TL,                                     \
    10        ::boost::test_tools::tt_detail::CT                                      \
    11        BOOST_JOIN( BOOST_TEST_TOOL_PASS_ARGS, frwd_type )( ARGS ) );           \
    12} while( ::boost::test_tools::tt_detail::dummy_cond() )                         \
    

    maflcko commented at 2:55 pm on November 25, 2024:

    theuni commented at 6:52 pm on November 25, 2024:

    NOLINT comments are easy searchable and effectively document the issues that have to be resolved.

    Sorry, but no, that is exactly the opposite of what NOLINT documents!

    NOLINT means “I’m smarter than my static analyzer”.

    TODO would effectively document an issue to be resolved.

    Hard NACK here due to the reasoning being applied. In fact, it rises to the level of suspicion. I’ll be reviewing all of these refactors in great detail.


    l0rinc commented at 7:07 pm on November 25, 2024:
    @hebasto, what if we invert this and push the fixes in separate PRs and only merge this TIDY_LLVM_V upgrade after all problems were already resolved?

    hebasto commented at 8:00 pm on November 25, 2024:

    NOLINT comments are easy searchable and effectively document the issues that have to be resolved.

    Sorry, but no, that is exactly the opposite of what NOLINT documents!

    NOLINT means “I’m smarter than my static analyzer”.

    TODO would effectively document an issue to be resolved.

    Hard NACK here due to the reasoning being applied. In fact, it rises to the level of suspicion. I’ll be reviewing all of these refactors in great detail.

    A bare NOLINT, without any explanation, has never meant “I’m smarter than my static analyzer”β€”at least, not to me.


    theuni commented at 8:11 pm on November 25, 2024:

    A bare NOLINT, without any explanation, has never meant “I’m smarter than my static analyzer”β€”at least, not to me.

    I’m not sure what else it could mean.

    From the docs: “clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning”

    Does this mean there are other places in the code where NOLINT is used as a means of documenting a todo? If so, we need to address those asap.


    hebasto commented at 8:14 pm on November 25, 2024:

    I’m not sure what else it could mean.

    From the docs: “clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning”

    You are right.


    theuni commented at 8:33 pm on November 25, 2024:

    @hebasto No need to close this if we can change the approach. I think the changes are useful (once the actual bugs are fixed), but your suggestion that other real bugs might have been previously masked set off my alarm bells. Looking through the code now (bench/tests excluded), I don’t see any other examples of this.

    I think we can agree on an approach where we (in this order):

    • Fix the actual bugs
    • Notate the false-positives
    • Enable the checks.

    It sounds like it also is worth adding a line in developer-notes.md that describes when a NOLINT is/is not appropriate.


    maflcko commented at 8:29 am on November 27, 2024:

    My preference would be to drop this comment blob. Otherwise, there will be bike-shedding when it is fine to remove it. Also, the code has to be touched again at that time. It seems better to just touch it once and then be done with it.

    Suggested diff:

     0diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
     1index 6a23a7b..5e18d23 100644
     2--- a/src/test/result_tests.cpp
     3+++ b/src/test/result_tests.cpp
     4@@ -63,7 +63,8 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
     5 {
     6     ExpectResult(result, true, str);
     7     BOOST_CHECK_EQUAL(result.has_value(), true);
     8-    BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
     9+T expected{std::forward<Args>(args)...};
    10+    BOOST_CHECK_EQUAL(result.value(), expected);
    11     BOOST_CHECK_EQUAL(&result.value(), &*result);
    12 }
    13 
    

    (and then possibly mention the comment in the commit message)

  22. hebasto closed this on Nov 25, 2024

  23. ci: Update Clang in "tidy" job
    This change switches to the latest IWYU 0.23, which is compatible with
    Clang 19.
    
    New "bugprone-use-after-move" and "modernize-use-starts-ends-with"
    warnings that emerged have been addressed.
    0c1e3cfc6f
  24. iwyu: Drop backported mapping
    See: https://github.com/include-what-you-use/include-what-you-use/pull/1560
    44f8c9c876
  25. in src/test/coins_tests.cpp:777 in 74e7813987 outdated
    772@@ -773,8 +773,10 @@ static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount mo
    773 template <typename... Args>
    774 static void CheckAddCoin(Args&&... args)
    775 {
    776-    for (const CAmount base_value : {ABSENT, SPENT, VALUE1})
    777+    for (const CAmount base_value : {ABSENT, SPENT, VALUE1}) {
    778+        // NOLINTNEXTLINE(bugprone-use-after-move)
    


    theuni commented at 8:00 pm on November 25, 2024:

    As mentioned here, this change struck me as dangerous.

    It shouldn’t be marked nolint, rather it should be fixed: drop the forward and pass by const Args&.

    I understand the desire to turn on new checks and notate the false-positives, but the result should always be better/safer/cleaner code.

    In this case, the result would’ve been a latent bug explicitly marked notabug. And while a buggy test isn’t the end of the world, the suggestion that the approach for recent similar PR’s might have been “quiet the static analysis tool first, fix the bug later” struck me as so backwards that I had to wonder if any other actual bugs in more important code had been masked as well. Looking through recent/current PRs, I don’t see any other examples of that (modulo the boost issue pointed out here).

    Please prioritize correctness and consider what you’re signaling to reviewers.


    hebasto commented at 8:03 pm on November 25, 2024:
    Right.

    hebasto commented at 8:08 pm on November 25, 2024:

    As mentioned here, this change struck me as dangerous.

    My apologies.

    An additional context’s available here: #31306 (review).

  26. hebasto reopened this on Nov 26, 2024

  27. hebasto force-pushed on Nov 26, 2024
  28. hebasto commented at 11:04 pm on November 26, 2024: member

    No need to close this if we can change the approach.

    Rebased on top of the merged #31305 and #31364.

    I think we can agree on an approach where we (in this order):

    • Fix the actual bugs

    • Notate the false-positives

    • Enable the checks.

    Done.

    It sounds like it also is worth adding a line in developer-notes.md that describes when a NOLINT is/is not appropriate.

    Left this change for a follow-up.


github-metadata-mirror

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-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me