bitcoin-tidy clang-tidy plugin
#26296
Demo of integrating the bitcoin-tidy, clang-tidy plugin written by theuni into our tidy CI job.
The plugin currently has a single check, bitcoin-unterminated-logprintf. This would replace our current Python driven, git-grep-based, .cpp file only, lint-logs linter.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theuni, MarcoFalke, TheCharlatan |
| Concept ACK | dergoegge |
| Approach ACK | stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
If this requires clang-15, I’d prefer to wait until that is backported to Ubuntu Jammy instead of hopping the short-term releases.
This doesn’t actually require LLVM 15, it’s just that the run-clang-tidy script shipped with LLVM 15 has support for passing -load arguments through to clang-tidy.
I’ve dropped the Kinetic / LLVM 15 bump, in favour of a different approach; patching -load support into the llvm 14 run-clang-tidy script.
Concept ACK (obviously). Thought dump below.
A few things worth considering when it comes to these plugins:
Elaborating:
c++ is complicated, so defining compiler rules is also complicated. Taking this code for a trivial example:
0struct A{};
1void func()
2{
3 A foo;
4 const A bar;
5}
In testing, one may find that the following query works to find all declarations of type A:
varDecl(hasType(asString("struct A")))
However, this will find only the non-const-qualified foo and not bar.
Instead, we’d want something more like: varDecl(hasType(cxxRecordDecl(hasName("A"))))
I’m mentioning this to try to demonstrate what I meant by “they do exactly what you tell them to”. It’s quite common (especially when just getting started) to end up with an overly-broad or unnecessarily specific query. The first query would appear to be fine if a const A is not used anywhere. The best defense against this from what I can tell is adding a test for each failing corner-case.
So I think we’ll likely want some pretty thorough documentation that states exactly what the check is intended to do. Additionally, we’ll want tests that verify those intentions. In the case of the plugin here, I have added some loose checks here but nothing automated: https://github.com/theuni/bitcoin-tidy/blob/main/test_desig_init.cpp . An example of a corner-case that wasn’t caught at first with this check is included here: nested structs with uninitialized members.
tl;dr: before merge, I think we’ll want to add a mini test suite to the plugin repo that allows us to verify that checks are working as defined/intended. Additionally, there will need to be some understanding that checks will likely require refinement over time as new corner cases are exposed.
- We may want different checks for certain parts of the code.
This isn’t important for now, but I suspect that the infrastructure for this will end up being more complicated eventually. The obvious outlier (imo) is libbitcoinkernel, where we may want to add more strict checks that other parts of the code (bitcoin-qt for ex) aren’t required to abide by.
Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .
Edit: I guess one is for constructors, the other is for aggregates, so they are orthogonal, and go hand-in-hand.
See also #26762 (comment)
Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .
Last I checked, turning that on was essentially unusable, due to false-positives. Will take another look.
SetNull method, no? In those cases the SetNull method can be removed, or the initializers duplicated from the method?
Reopened as I’d like to get the infrastructure for this hooked up.
I think the issue here was disagreement about what the initial plugin should do. @MarcoFalke Do you have any suggestions for an uncontroversial “hello world” type plugin just to get started with something?
\n check for logging? Also suggested in https://github.com/bitcoin/bitcoin/issues/27825
Nice, that one’s already done :)
I’ll update it and we’ll start with that one instead.
Updated to use the new plugin, https://github.com/theuni/bitcoin-tidy-plugin, and new check bitcoin-unterminated-logprintf.
Updated the PR description.
lint-logs.py can probably be deleted?
lint-logs.py can probably be deleted?
Added a commit to drop it.
74@@ -75,6 +75,12 @@ if [[ "${RUN_TIDY}" == "true" ]]; then
75 git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
76 cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
77 make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"
78+
79+ if [ ! -d "${DIR_CORE_TIDY}" ]; then
80+ git clone --depth=1 https://github.com/theuni/bitcoin-tidy-plugin "${DIR_CORE_TIDY}"
The CI system caches built images. The cache-key is (among other stuff) this file. If the file does not change, the cache-key does not change either. However, if the bitcoin-tidy-plugin changes, everyone may have a different version of bitcoin-tidy-plugin in the cache, without the cache being invalidated. Thus, everyone may get different results for the same commit-id in this repo. (This is known as non-deterministic).
It can be fixed by pinning to a tag/commit/branch/…
git checkout 1806ef33ff8b14256d766eb9f616a3528aad4464 in the next line. (Obviously this will break due to --depth=1 if another commit is pushed on top of it)
Also, CI (tidy) fails
Obviously. Given there is a commit here to make it fail? Any other issues are outlined in the PR description. The failures we’d care about currently are the actual integration / build failing, but everything is working as expected.
Obviously. Given there is a commit here to make it fail?
Yes but it’s also failing on a different LogPrint statement, so no need for that commit i guess
Yes but it’s also failing on a different LogPrint statement, so no need for that commit i guess
Yea, assuming it’s an actual issue, and not a false positive. If it is an issue, then it means our current linter doesn’t work anyways.
(I added a full list of these to the PR description before pushing the new changes).
Yea, assuming it’s an actual issue, and not a false positive.
At least for the failure in ./test/util/chainstate.h it’s because the current linter only works on cpp files not headers.
Fixed the non-determinism. Dropped the test commit. Fixed the issue in chainstate.h.
it’s because the current linter only works on cpp files not headers.
I guess that’s another potential +1 for this approach?
/* Continued */ code-bloat?
74@@ -75,6 +75,13 @@ if [[ "${RUN_TIDY}" == "true" ]]; then
75 git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
76 cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
77 make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"
78+
79+ if [ ! -d "${DIR_CORE_TIDY}" ]; then
80+ git clone https://github.com/theuni/bitcoin-tidy-plugin "${DIR_CORE_TIDY}"
81+ git -C "${DIR_CORE_TIDY}" checkout 1806ef33ff8b14256d766eb9f616a3528aad4464
Should this also remove the /* Continued */ code-bloat?
Will add this.
Back to draft while we look at an in-tree approach.
Heh, really sorry @fanquake .
I think the out-of-tree approach makes sense for (for example) plugins where we can create attributes and assign them meaning. In that case, the “atomicity” issue goes away, because the code either respects the attribute or it doesn’t.
But yeah, now that @MarcoFalke mentions it, that approach is cumbersome when we have to worry about things getting out of sync. So let’s see how it looks to do it in-tree. @fanquake Want to keep going here, or shall I open a new PR?
I guess my only real question is: if we’re going to keep it in-tree does it need a full CMake buildsytem as opposed to a simple Makefile? It’s kinda overkill for now, but works well and doesn’t appear to complicate the c-i in any way. So I guess I’m inclined to leave it unless anyone objects?
Reworked. I’m guessing some of our linters could fail against the new lint code? If so, I think we should just exclude it from them. Added some more fixups. Updated the PR description. Not 100% sure about the CI (build) dir handling, but this can also be improved.
Added a commit to drop the /* Continued */ markers. Can be squashed into the previous commit which drops lint-logs.
edit: added contrib/devtools/bitcoin-tidy as an exclusion to the header and include linters.
0@@ -0,0 +1,9 @@
1+# Bitcoin Tidy
2+
3+Example Usage:
4+
5+```bash
6+cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release
7+make -j -C build
nit:
0make -j$(nproc) -C build
12+AST_MATCHER(clang::StringLiteral, unterminated) {
13+ size_t len = Node.getLength();
14+ if(len == 0) {
15+ return false;
16+ }
17+ if(Node.getCodeUnit(len-1) == '\n') {
src/.clang-format.
147@@ -148,9 +148,12 @@ if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
148 fi
149
150 if [ "${RUN_TIDY}" = "true" ]; then
151+ cmake -B "${DIR_CORE_TIDY}"/build -DLLVM_DIR=/usr/lib/llvm-16/cmake -DCMAKE_BUILD_TYPE=Release -S "${BASE_ROOT_DIR}"/contrib/devtools/bitcoin-tidy
152+ cmake --build "${DIR_CORE_TIDY}"/build "$MAKEJOBS"
32+{
33+ LogPrintf("hello world!\n");
34+}
35+void good_func2()
36+{
37+ LogPrintf("hello world!...");
As you said, this one isn’t a big deal but it does highlight the general issue of exceptions.
Generally I see a few ways to handle them:
LogPrintf_nonewline() and using as necessaryrun-clang-tidy-16 -quiet -extra-arg="-DBITCOIN_TIDY" -load=...In this case I think our best options are the first or the last.
/* TIDY IGNORE LOG */ (or similar comment) in the source code and then skip those with the plugin?
* Try to find a preprocessor guard that applies only to clang-tidy. I can't find one but I assume I'm just missing it.
Whoops, here it is: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics
Neat. So the exceptions (if it works as documented) can become:
0LogPrintf("foo..."); // NOLINT(bitcoin-unterminated-logprintf)
Which is nice and self-documenting :)
Pushed a commit here which tests this: https://github.com/theuni/bitcoin-tidy-plugin/commit/cdc007022b6cd40320b7e74ab4cbb8e67a6d6e17
It works as expected.
You could just add a
/* TIDY IGNORE LOG */(or similar comment) in the source code and then skip those with the plugin?
Just to address this as I agree it’d be a reasonable solution, there’s (currently at least) no access to comments from clang-tidy plugins as they’re not part of the AST.
Sorry for all the spam here, one more.
Given the nice opt-out above, I’ve pushed changes to my repo which eliminate all exceptions, so the plugin rule is now very simply: “every logprintf format string must end in a newline”. That will mean we need to opt-out of the current “…” uses with // NOLINT(bitcoin-unterminated-logprintf), which I think is the correct thing to do.
Pushed a commit here which tests this: theuni/bitcoin-tidy-plugin@cdc0070
It works as expected.
Nice. Maybe include this here?
Imo these tests are buggy and are correctly failing here.
test/logging_tests.cpp:86:77: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors] LogPrintf_(“fn1”, “src1”, 1, BCLog::LogFlags::NET, BCLog::Level::Debug, “foo1: %s”, “bar1\n”);
If we don’t enforce that the newline is in the format-string, it’s really nasty to enforce that it’s “somewhere in one of the arguments”, as that could even be added to a string at runtime.
As far as I can tell the only place this is used is in these tests, so I’d suggest just deleting them so that we can enforce the rule sanely.
64@@ -65,6 +65,7 @@ export BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build}
65 # This folder exists only on the ci guest, and on the ci host as a volume.
66 export PREVIOUS_RELEASES_DIR=${PREVIOUS_RELEASES_DIR:-$BASE_ROOT_DIR/releases/$HOST}
67 export DIR_IWYU="${BASE_SCRATCH_DIR}/iwyu"
68+export DIR_CORE_TIDY="${BASE_SCRATCH_DIR}/tidy"
export DIR_IWYU="${BASE_SCRATCH_DIR}/iwyu") to the only test env that needs it (ci/test/00_setup_env_native_tidy.sh). Otherwise it seems odd to export it to all test envs, even if they don’t need it?
DIR_CORE_TIDY entirely.
DIR_IWYU.
DIR_CORE_TIDY and just used BASE_SCRATCH_DIR directly.
Added example_logprintf.cpp to the lint-format-strings.py exclusion list.
As far as I can tell the only place this is used is in these tests, so I’d suggest just deleting them so that we can enforce the rule sanely.
Actually since they test LogPrintf_ directly, I suggest just fixing up the newline part so that we no longer support that construction.
0diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
1index 2699d316da..e448805e69 100644
2--- a/src/test/logging_tests.cpp
3+++ b/src/test/logging_tests.cpp
4@@ -83,10 +83,10 @@ BOOST_AUTO_TEST_CASE(logging_timer)
5 BOOST_FIXTURE_TEST_CASE(logging_LogPrintf_, LogSetup)
6 {
7 LogInstance().m_log_sourcelocations = true;
8- LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s", "bar1\n");
9- LogPrintf_("fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::None, "foo2: %s", "bar2\n");
10- LogPrintf_("fn3", "src3", 3, BCLog::LogFlags::NONE, BCLog::Level::Debug, "foo3: %s", "bar3\n");
11- LogPrintf_("fn4", "src4", 4, BCLog::LogFlags::NONE, BCLog::Level::None, "foo4: %s", "bar4\n");
12+ LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s\n", "bar1");
13+ LogPrintf_("fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::None, "foo2: %s\n", "bar2");
14+ LogPrintf_("fn3", "src3", 3, BCLog::LogFlags::NONE, BCLog::Level::Debug, "foo3: %s\n", "bar3");
15+ LogPrintf_("fn4", "src4", 4, BCLog::LogFlags::NONE, BCLog::Level::None, "foo4: %s\n", "bar4");
16 std::ifstream file{tmp_log_path};
17 std::vector<std::string> log_lines;
18 for (std::string log; std::getline(file, log);) {
DIR_IWYU.
Updated logging tests with the above suggestion.
71@@ -72,9 +72,9 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
72 fi
73
74 if [[ "${RUN_TIDY}" == "true" ]]; then
75- git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
76- cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
77- make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"
78+ git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${BASE_SCRATCH_DIR}"/include-what-you-use
79+ cmake -B "${BASE_SCRATCH_DIR}"/iwyu-build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${BASE_SCRATCH_DIR}"/include-what-you-use
80+ make -C "${BASE_SCRATCH_DIR}"/iwyu-build/ install "$MAKEJOBS"
0 git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 /include-what-you-use
1 cmake -B /iwyu-build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S /include-what-you-use
2 make -C /iwyu-build/ install "$MAKEJOBS"
nit: I wonder if this is better put directly under /? It would give the following (edge-case) benefits:
DANGER_RUN_CI_ON_HOST, it keeps the installed stuff persistent. Even after a git clean -dffx, which seems preferable, because the installation is only run once in any case.Feel free to ignore or adjust.
I think I only suggested to move the export: #26296 (review) (not remove it).
On a second thought, I am wondering if it makes more sense to do something like #26296 (review) instead.
I mean anything that is correct is fine here, but in light of multiple alternatives, it seems odd to pick one, then revert it, and pick something else? (Sorry for coming up with the second alternative)
It is just a nit, so feel free to ignore. Just be aware that I may create the follow-up soon, which will then conflicting with this pull.
Thanks, looks like this is even a bugfix, see #28185 (comment)
Unrelated, for a future idea: Since iwyu is installed (/usr/local/bin/fix_includes.py, etc), we could even remove the /iwyu-build/ and /include-what-you-use/ folder to save some storage. But yeah, that’s for the future.
147@@ -148,23 +148,26 @@ if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
148 fi
149
150 if [ "${RUN_TIDY}" = "true" ]; then
151+ cmake -B "${BASE_SCRATCH_DIR}"/tidy-build -DLLVM_DIR=/usr/lib/llvm-16/cmake -DCMAKE_BUILD_TYPE=Release -S "${BASE_ROOT_DIR}"/contrib/devtools/bitcoin-tidy
152+ cmake --build "${BASE_SCRATCH_DIR}"/tidy-build "$MAKEJOBS"
Also, the commit message of the last commit is wrong?
What would you like to see adjusted?
What would you like to see adjusted?
I think the link to the external repo can be dropped, given that the source code lives in this repo, and linking to an outside one could increase confusion as to which place is the correct one to submit patches to?
I think the link to the external repo can be dropped,
Ok.
Also fixed the lint regex.
75@@ -76,7 +76,7 @@ class CBitcoinLevelDBLogger : public leveldb::Logger {
76
77 assert(p <= limit);
78 base[std::min(bufsize - 1, (int)(p - base))] = '\0';
79- LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base);
80+ LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s\n", base);
\n is already included in base.
To clarify, I am trying to say:
0 LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf)
6+
7+set(CMAKE_CXX_STANDARD 17)
8+set(CMAKE_CXX_STANDARD_REQUIRED True)
9+set(CMAKE_CXX_EXTENSIONS False)
10+
11+# TODO: Figure out how to avoid the terminfo check
Actually, looking around, I think it makes more sense to just delete this comment. Autotools checks for a bazillion things it doesn’t need. This is just one of those quirks that can’t be turned off.
(The issue is that LLVM’s CMake file checks for terminfo even though we don’t need it. It doesn’t seem to be a problem in the real world. If we encounter an actual issue, THEN we can worry about working around it.)
20+
21+#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
22+#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
23+
24+// Use a macro instead of a function for conditional logging to prevent
25+// evaluating arguments when logging for the category is not enabled.
0@@ -0,0 +1,91 @@
1+// Copyright (c) 2023 Bitcoin Developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+// Warn about any use of LogPrintf that does not end with a newline.
This used to be part of the default build, but now needs to be run manually: make bitcoin-tidy-tests
It’s probably not a bad idea to have them run by c-i, that way we can see that they’re actually catching something.
It’s probably not a bad idea to have them run by c-i, that way we can see that they’re actually catching something.
Right, and also to ensure it compiles in the first place.
FWIW, I made this non-default because I don’t think it’s a good idea for a vanilla make to spew warnings on purpose. It’s bound to set off c-i somewhere eventually.
As @MarcoFalke points out though, we do want it run when we’re in control of the environment and not worried about those warnings. In order to run them, I suggest:
0diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
1index 75d5469267..02358db789 100755
2--- a/ci/test/06_script_b.sh
3+++ b/ci/test/06_script_b.sh
4@@ -150,6 +150,7 @@ fi
5 if [ "${RUN_TIDY}" = "true" ]; then
6 cmake -B /tidy-build -DLLVM_DIR=/usr/lib/llvm-16/cmake -DCMAKE_BUILD_TYPE=Release -S "${BASE_ROOT_DIR}"/contrib/devtools/bitcoin-tidy
7 cmake --build /tidy-build "$MAKEJOBS"
8+ cmake --build /tidy-build --target bitcoin-tidy-tests "$MAKEJOBS"
9
10 set -eo pipefail
11 cd "${BASE_BUILD_DIR}/bitcoin-$HOST/src/"
ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b 🗑
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b 🗑
36uebzqjnb7gbD/W0sPwdA9sB69ba9mpiZf0I48jxsqT/O0696vGO6XlfTu0E0KtjlUtVcYHcTJhwzmC2ysqoCg==
Approach ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b
Did a code review too but both the build stuff as well as the clang-tidy syntax is still very new to me so doesn’t mean too much.
Are the bitcoin-tidy tests being run in CI with this PR? And perhaps some tidy-test (in example_logprintf.cpp) for multiline strings would be helpful too?
NOLINTs hooked up, and yet c-i is still passing. Beyond the one that @MarcoFalke pointed out above, surely more are still needed right?
@fanquake Would you mind removing the single use of NOLINT and pushing, just so that we can make sure the c-i still fails as expected after all the changes?
Assuming it does, LGTM either with or without addressing this comment.
Would you mind removing the single use of NOLINT and pushing, just so that we can make sure the c-i still fails as expected after all the changes?
Done.
From: https://cirrus-ci.com/task/6097246463197184
0[100%] Building CXX object CMakeFiles/bitcoin-tidy-tests.dir/example_logprintf.cpp.o
1/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:65:15: warning: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf]
2 LogPrintf("hello world!");
3 ^
4 \n
5/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:22:68: note: expanded from macro 'LogPrintf'
6#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
7 ^
8/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:21:104: note: expanded from macro 'LogPrintLevel_'
9#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
10 ^
11/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:69:15: warning: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf]
12 LogPrintf("");
13 ^
14 \n
15/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:22:68: note: expanded from macro 'LogPrintf'
16#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
17 ^
18/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:21:104: note: expanded from macro 'LogPrintLevel_'
19#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
20 ^
21/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:74:15: warning: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf]
22 LogPrintf("hello world!...");
23 ^
24 \n
25/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:22:68: note: expanded from macro 'LogPrintf'
26#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
27 ^
28/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:21:104: note: expanded from macro 'LogPrintLevel_'
29#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
30 ^
31[100%] Built target bitcoin-tidy-tests
0clang-tidy-16 -p=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/dbwrapper.cpp
1dbwrapper.cpp:79:68: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
2 LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base);
3 ^
4 \n
5./logging.h:256:45: note: expanded from macro 'LogPrintLevel'
6 LogPrintLevel_(category, level, __VA_ARGS__); \
7--
Nice, thanks!
Enable `bitcoin-unterminated-logprintf`.
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 👠
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 👠
3AUAtgJBmTgwGLADs8IgqfpOQg2nQfQ2+D/7P8lwnKkrIGJwTNAjXOYnonfN9oAT1L5+CTeUJzKaPpGHVi5t7Cw==
is anything left to be done here
I missed that Cory had already ACK’d, and was waiting for him to re-ACK. Given that’s already happened, I don’t think there’s anything left here, and docs can be fixed in the followup.
0@@ -0,0 +1,8 @@
1+# Bitcoin Tidy
2+
3+Example Usage:
4+
5+```bash
6+cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release
DLLVM_DIR should be for them?
43+ */
44+ finder->addMatcher(
45+ cxxMemberCallExpr(
46+ thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))),
47+ callee(cxxMethodDecl(hasName("WalletLogPrintf"))),
48+ hasArgument(0, stringLiteral(unterminated()).bind("logstring"))),
WalletLogPrintf is never a string literal (the parameter is).
41+ wallet.WalletLogPrintf("foo");
42+ wallet->WalletLogPrintf("foo");
43+ */
44+ finder->addMatcher(
45+ cxxMemberCallExpr(
46+ thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))),
This is wrong, too. ScriptPubKeyMan has a log statement, too.
My recommendation would be to just remove this line completely, unless there is a reason to have it. The other lines should be exact enough to match everything that is needed, without under- or over-matching.
Mind creating a pull with the outstanding feedback? :)
If not, I’ll try to do it next week.