Enable some bugprone clang-tidy checks, which seem useful.
Apart from a test-only bug, they didn't find anything severe, but I think it still makes sense to enable them, so that it is easier to read the resulting code. Also, it is trivial to disable them, if they turn out annoying.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto, stickies-v |
| Stale ACK | hodlinator |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/39580996840/job/119158281831</sub>
<sub>LLM reason (✨ experimental): The build failed because Clang treated the unused copy_target variable in util_not_null_tests.cpp as an error (-Werror).</sub>
<details><summary>Hints</summary>
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.
</details>
Enable some bugprone clang-tidy checks...
Concept ACK.
The code change does not change any behavior, because the name is not
used in the compile-time context.
178 | @@ -179,7 +179,7 @@ void WalletController::removeAndDeleteWallet(WalletModel* wallet_model) 179 | // Unregister wallet model. 180 | { 181 | QMutexLocker locker(&m_mutex); 182 | - m_wallets.erase(std::remove(m_wallets.begin(), m_wallets.end(), wallet_model)); 183 | + std::erase(m_wallets, wallet_model);
fa077e9 refactor: Enable bugprone-inaccurate-erase:
What do you mean by this?
Note: Wallets are unique, so the code change does not change any behavior.
https://en.cppreference.com/cpp/container/vector/erase2 seems to specify that they are equivalent.
https://en.cppreference.com/cpp/container/vector/erase2 seems to specify that they are equivalent.
Heh, the docs say that
std::erase(c,v);
is equivalent to
c.erase(std::remove(c.begin(), c.end(), value), c.end());
However, the code here is:
m_wallets.erase(std::remove(m_wallets.begin(), m_wallets.end(), wallet_model)); // note the "missing" end
This is harmless in this case, but confusing, because:
So my recommendation would be to completely get rid of std::vector::erase and instead use the C++20 std::erase(vector,...)
Dang, missed it. :D
nit: The commit message could be made clearer:
Note: Wallets are unique, so even through we previously weren't specifying a range of iterators to vector::erase(), the behavior does not change.
nit: The commit message could be made clearer:
Thx, will leave this thread open for now, so that reviewers see it, and so that I can avoid a push and review churn.
put the follow-up in https://github.com/bitcoin/bitcoin/pull/36682
Concept ACK fa60b2ae8db2072f5a851f7f9669cb6606fb7806
ACK fa60b2ae8db2072f5a851f7f9669cb6606fb7806
ACK fa60b2ae8db2072f5a851f7f9669cb6606fb7806
Also, fix some test bugs.
Also, instead of using the erase-remove idiom consistenly, just use the
C++20 std::erase here.
Note: Wallets are unique, so the code change does not change any
behavior.
Also, sort
Whenever the check fires, a missing LIFETIMEBOUND should be added.
1173 | @@ -1174,7 +1174,7 @@ BOOST_AUTO_TEST_CASE(test_Capitalize) 1174 | { 1175 | BOOST_CHECK_EQUAL(Capitalize(""), ""); 1176 | BOOST_CHECK_EQUAL(Capitalize("bitcoin"), "Bitcoin"); 1177 | - BOOST_CHECK_EQUAL(Capitalize("\x00\xfe\xff"), "\x00\xfe\xff"); 1178 | + BOOST_CHECK_EQUAL(Capitalize("\x00\xfe\xff"s), "\x00\xfe\xff"sv);
note: it seems like string-literal-with-embedded-nul only checks std::string constructors, not std::string_view. Similar ToLower (:1156) and ToUpper (:1170) tests have the same bug and don't get flagged. For example, the below diff passes the test.
<details> <summary>git diff on fa60b2ae8d</summary>
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 6db2f90908..f3cdec9063 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1153,7 +1153,7 @@ BOOST_AUTO_TEST_CASE(test_ToLower)
BOOST_CHECK_EQUAL(ToLower(""), "");
BOOST_CHECK_EQUAL(ToLower("#HODL"), "#hodl");
- BOOST_CHECK_EQUAL(ToLower("\x00\xfe\xff"), "\x00\xfe\xff");
+ BOOST_CHECK_EQUAL(ToLower("\x00\xfe\xff"), "");
}
BOOST_AUTO_TEST_CASE(test_ToUpper)
@@ -1167,7 +1167,7 @@ BOOST_AUTO_TEST_CASE(test_ToUpper)
BOOST_CHECK_EQUAL(ToUpper(""), "");
BOOST_CHECK_EQUAL(ToUpper("#hodl"), "#HODL");
- BOOST_CHECK_EQUAL(ToUpper("\x00\xfe\xff"), "\x00\xfe\xff");
+ BOOST_CHECK_EQUAL(ToUpper("\x00\xfe\xff"), "");
}
BOOST_AUTO_TEST_CASE(test_Capitalize)
</details>
thx, done
ACK facb2209061ce40f558d9e1feaaf8e9a627117e5.
""sv in util_tests.cpp needs #include <string_view>.
re-ACK facb2209061ce40f558d9e1feaaf8e9a627117e5
""svinutil_tests.cppneeds#include <string_view>.
thx, will leave this for iwyu to fix, to limit the review churn here. (The include exists indirectly, so this is just a iwyu style issue)