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
ACK	hebasto, l0rinc
Concept ACK	kevkevinpal

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

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 outdated

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 outdated

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 outdated

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 outdated

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.

fanquake force-pushed on Apr 17, 2025

fanquake commented at 12:43 pm on April 17, 2025: member

@l0rinc I’ve taken some of these, but I’m not planning on making further refactors here, just updating the infra. Happy to review a followup.

l0rinc commented at 12:58 pm on April 17, 2025: contributor

ACK e1254c4bd99441ed954480a5883132786ac3c36a

 0diff --git a/src/common/args.cpp b/src/common/args.cpp
 1index ab84d32722..1e3f6d1b88 100644
 2--- a/src/common/args.cpp
 3+++ b/src/common/args.cpp
 4@@ -189,7 +189,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
 5         // internet" warning, and clicks the Open button, macOS passes
 6         // a unique process serial number (PSN) as -psn_... command-line
 7         // argument, which we filter out.
 8-        if (key.substr(0, 5) == "-psn_") continue;
 9+        if (key.starts_with("-psn_")) continue;
10 #endif
11 
12         if (key == "-") break; //bitcoin-tx using stdin
13diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
14index 994ab73e72..f8cdd4ce08 100644
15--- a/src/node/blockstorage.cpp
16+++ b/src/node/blockstorage.cpp
17@@ -624,7 +624,7 @@ void BlockManager::CleanupBlockRevFiles() const
18         const std::string path = fs::PathToString(it->path().filename());
19         if (fs::is_regular_file(*it) &&
20             path.length() == 12 &&
21-            path.substr(8,4) == ".dat")
22+            path.ends_with(".dat"))
23         {
24             if (path.starts_with("blk")) {
25                 mapBlockFiles[path.substr(3, 5)] = it->path();
26diff --git a/src/qt/test/rpcnestedtests.cpp b/src/qt/test/rpcnestedtests.cpp
27index 0797d31a71..eb74ef8559 100644
28--- a/src/qt/test/rpcnestedtests.cpp
29+++ b/src/qt/test/rpcnestedtests.cpp
30@@ -64,13 +64,13 @@ void RPCNestedTests::rpcNestedTests()
31     RPCConsole::RPCExecuteCommandLine(m_node, result, "getblock( getblock( getblock(getbestblockhash())[hash] )[hash], true)"); //4 level nesting with whitespace, filtering path and boolean parameter
32 
33     RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo");
34-    QVERIFY(result.substr(0,1) == "{");
35+    QVERIFY(result.starts_with("{"));
36 
37     RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()");
38-    QVERIFY(result.substr(0,1) == "{");
39+    QVERIFY(result.starts_with("{"));
40 
41     RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo "); //whitespace at the end will be tolerated
42-    QVERIFY(result.substr(0,1) == "{");
43+    QVERIFY(result.starts_with("{"));
44 
45     RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()[\"chain\"]"); //Quote path identifier are allowed, but look after a child containing the quotes in the key
46     QVERIFY(result == "null");

DrahtBot requested review from hebasto on Apr 17, 2025

l0rinc approved

fanquake force-pushed on Apr 17, 2025

DrahtBot added the label CI failed on Apr 17, 2025

DrahtBot commented at 4:12 pm on April 17, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40724753704

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

l0rinc commented at 4:14 pm on April 17, 2025: contributor

Was modernize-type-traits removed from src/.clang-tidy in latest push on purpose?

fanquake commented at 4:15 pm on April 17, 2025: member

Yes.

DrahtBot removed the label CI failed on Apr 17, 2025

refactor: starts/ends_with changes for clang-tidy 20 2b85d31bcc

ci: clang-tidy 20 08aa7fe232

fanquake force-pushed on Apr 22, 2025

hebasto approved

hebasto commented at 12:58 pm on April 22, 2025: member

ACK 08aa7fe2326391e6d633c2da50959754e3e7b8d6.

This PR effectively bumps two tools: clang-tidy and include-what-you-use. Release notes for the latter are available here.

DrahtBot requested review from l0rinc on Apr 22, 2025

l0rinc commented at 10:42 am on April 23, 2025: contributor

ACK 08aa7fe2326391e6d633c2da50959754e3e7b8d6

Since last review modernize-type-traits lint rule was removed, and RemovePrefixView was also updated to use starts_with.

 0diff --git a/src/.clang-tidy b/src/.clang-tidy
 1index aa171dc0ac..1cf270833a 100644
 2--- a/src/.clang-tidy
 3+++ b/src/.clang-tidy
 4@@ -9,7 +9,6 @@ bugprone-lambda-function-name,
 5 bugprone-unhandled-self-assignment,
 6 misc-unused-using-decls,
 7 misc-no-recursion,
 8-modernize-type-traits,
 9 modernize-use-default-member-init,
10 modernize-use-emplace,
11 modernize-use-equals-default,
12diff --git a/src/util/string.h b/src/util/string.h
13index 3075e46abb..4b87525627 100644
14--- a/src/util/string.h
15+++ b/src/util/string.h
16@@ -168,7 +168,7 @@ std::vector<T> Split(const std::span<const char>& sp, char sep)
17 
18 [[nodiscard]] inline std::string_view RemovePrefixView(std::string_view str, std::string_view prefix)
19 {
20-    if (str.substr(0, prefix.size()) == prefix) {
21+    if (str.starts_with(prefix)) {
22         return str.substr(prefix.size());
23     }
24     return str;

hebasto merged this on Apr 23, 2025

hebasto closed this on Apr 23, 2025

fanquake deleted the branch on Apr 23, 2025

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`