ci: switch to LLVM 20 in tidy job

fanquake commented at 0:00 am on April 6, 2025: member

Switch to LLVM 20 in the tidy job.

DrahtBot commented at 0:00 am on April 6, 2025: 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/32226.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	kevkevinpal, hebasto

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:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot added the label Tests on Apr 6, 2025

kevkevinpal commented at 1:16 am on April 6, 2025: contributor

Concept ACK

I manually reviewed the tidy changes and they looked good to me. I think upgrading to LLVM 20 is a good upgrade

Looks like CI / test each commit is failing but seems unrelated to the changes made

kevkevinpal commented at 1:29 am on April 6, 2025: contributor

The reason why the CI / test each commit is failing

0(PR32226) bitcoin $ git merge --no-commit origin/master
1Already up to date.
2(PR32226) bitcoin $ git merge --abort
3fatal: There is no merge to abort (MERGE_HEAD missing).

tidy: add modernize-type-traits

Followup to #31904.

f208fb4ec2

refactor: starts_with changes for clang-tidy 20 20e62e84bf

ci: clang-tidy 20 7ea82809ea

in src/bitcoin-cli.cpp:1223 in 806fbffb54 outdated

1219@@ -1220,7 +1220,7 @@ static int CommandLineRPC(int argc, char *argv[])
1220         if (gArgs.GetBoolArg("-stdinwalletpassphrase", false)) {
1221             NO_STDIN_ECHO();
1222             std::string walletPass;
1223-            if (args.size() < 1 || args[0].substr(0, 16) != "walletpassphrase") {

l0rinc commented at 9:21 am on April 6, 2025:

nit: These changes seem related to the ones in #31306 (might be worth mentioning in the comment or PR description like we did with the previous commit)

fanquake force-pushed on Apr 6, 2025

in src/common/config.cpp:68 in 7ea82809ea

64@@ -65,7 +65,7 @@ static bool GetConfigOptions(std::istream& stream, const std::string& filepath,
65                 }
66             } else {
67                 error = strprintf("parse error on line %i: %s", linenr, str);
68-                if (str.size() >= 2 && str.substr(0, 2) == "no") {
69+                if (str.size() >= 2 && str.starts_with("no")) {

l0rinc commented at 9:31 am on April 6, 2025:

If it starts with “no” we already know that str.size() >= 2. This seems like a premature optimization to avoid doing the substring - but regardless, I don’t think it’s needed:

0                if (str.starts_with("no")) {

in src/core_read.cpp:86 in 7ea82809ea

82@@ -83,7 +83,7 @@ CScript ParseScript(const std::string& s)
83             }
84 
85             result << num.value();
86-        } else if (w.substr(0, 2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin() + 2, w.end()))) {
87+        } else if (w.starts_with("0x") && w.size() > 2 && IsHex(std::string(w.begin() + 2, w.end()))) {

l0rinc commented at 9:34 am on April 6, 2025:

Since IsHex can safely handle empty strings, we could simplify this one as well:

0        } else if (w.starts_with("0x") && IsHex(std::string(w.begin() + 2, w.end()))) {

proof:

0BOOST_AUTO_TEST_CASE(core_read)
1{
2    std::string w{"0x"};
3    const bool with_size_check{w.substr(0, 2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin() + 2, w.end()))};
4    const bool without_size_check{w.starts_with("0x") && IsHex(std::string(w.begin() + 2, w.end()))};
5    BOOST_CHECK_EQUAL(with_size_check, without_size_check);
6}

in src/node/blockstorage.cpp:627 in 7ea82809ea

625@@ -626,9 +626,9 @@ void BlockManager::CleanupBlockRevFiles() const
626             path.length() == 12 &&
627             path.substr(8,4) == ".dat")

l0rinc commented at 9:48 am on April 6, 2025:

since we’ve already checked that the length has to be 8+4, we might as well match the end here:

0            path.ends_with(".dat"))

in src/common/args.cpp:88 in 7ea82809ea

84@@ -85,7 +85,7 @@ KeyInfo InterpretKey(std::string key)
85         result.section = key.substr(0, option_index);
86         key.erase(0, option_index + 1);
87     }
88-    if (key.substr(0, 2) == "no") {
89+    if (key.starts_with("no")) {

l0rinc commented at 9:54 am on April 6, 2025:

I knew that the first parameter would throw if it’s out of rage, but wasn’t sure what substr did for shorter length strings (since I know starts_with would work for empty string, but 2 would be out of range), but the doc states:

len Number of characters to include in the substring (if the string is shorter, as many characters as possible are used).

And tried it and they both return false for empty or short string: 👍

l0rinc changes_requested

l0rinc commented at 10:10 am on April 6, 2025: contributor

I love it when new tidy checks are added, it avoids a lot of needless review churn!

It seems to me the current replacements enable a few other simplifications - and we can likely apply the same logic manually to a few places that weren’t found by tidy, e.g:

Can be replaced with `starts_with`

if (str.substr(0, 3) == std::string("OP")) { https://github.com/bitcoin/bitcoin/blob/master/src/core_write.cpp#L56
if (key.substr(0, 5) == "-psn_") continue; https://github.com/bitcoin/bitcoin/blob/master/src/common/args.cpp#L192
QVERIFY(result.substr(0,1) == "{"); https://github.com/bitcoin/bitcoin/blob/master/src/qt/test/rpcnestedtests.cpp#L67-L73
if (str.substr(0, prefix.size()) == prefix) { https://github.com/bitcoin/bitcoin/blob/master/src/util/string.h#L171

Can be replaced with `ends_with`

if (str.size() >= 2 && str.substr(str.size() - 2) == “==”) str.remove_suffix(2);` https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.cpp#L186-L188

hebasto commented at 1:01 pm on April 15, 2025: member

Concept ACK.

ci: switch to LLVM 20 in tidy job #32226

Code Coverage & Benchmarks

Reviews

Conflicts

Can be replaced with starts_with

Can be replaced with ends_with

Can be replaced with `starts_with`

Can be replaced with `ends_with`