scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

maflcko · 2021-11-18T20:00:33Z

Incorrect named args are source of bugs, like #22979.

To allow them being checked by clang-tidy, use a format it can understand.

DrahtBot · 2021-11-18T20:05:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23604 (Use Sock in CNode by vasild)
#23575 (fuzz: Rework FillNode by MarcoFalke)
#23507 (Refactor: Improve API design of ScriptToUniv, TxToUniv etc to return the UniValue instead of mutating a parameter/reference by mjdietzx)
#23465 (Remove CTxMemPool params from ATMP by lsilva01)
#23437 (refactor: AcceptToMemoryPool by lsilva01)
#23373 (Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change by vasild)
#23280 (init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths by dongcarl)
#22954 ([TESTS] Allow tx_invalid.json tests to include flag rules for if_unset: [A,B,C] then_unset: [D] by JeremyRubin)
#22924 (refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag by mjdietzx)
#22876 ([TESTS] Update Transaction Tests to permit setting a flag as always on and disabling the exhaustive failure test by JeremyRubin)
#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#22702 (Add allocator for node based containers by martinus)
#21878 (Make all networking code mockable by vasild)
#21160 (net/net processing: Move tx inventory into net_processing by jnewbery)

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.

katesalazar · 2021-11-18T20:29:07Z

Concept ACK, thanks.

Also, fix argument name for FastRandomContext.

-BEGIN VERIFY SCRIPT- perl -0777 -pi -e 's:((\(|\{|,)(\n| )*)\/\* ?([^=* ]+) ?\*\/ ?:\1/*\4=*/:g' $( git ls-files ./src/test ./src/wallet/test ) -END VERIFY SCRIPT-

maflcko · 2021-11-19T12:34:27Z

Setup clang-tidy:

apt install clang-tidy bear clang
./autogen.sh && ./configure --with-incompatible-bdb CC=clang CXX=clang++
make clean && bear make -j $(nproc)     # For bear 2.x
make clean && bear -- make -j $(nproc)  # For bear 3.x

Full test:

( cd ./src/ && run-clang-tidy  -j $(nproc) )

Diff test:

git diff | ( cd ./src/ && clang-tidy-diff -p2 -j $(nproc) )

shaavan

ACK fa00447

I independently recreated the changes done in this PR by:

Reseting the commit done in this PR and then,
Using the following command.

perl -0777 -pi -e 's:((\(|\{|,)(\n| )*)\/\* ?([^=* ]+) ?\*\/ ?:\1/*\4=*/:g' $( git ls-files ./src/test ./src/wallet/test ) src/test/crypto_tests.cpp src/test/util/setup_common.h

After that I compared the original PR and my recreation and I got the following diff:

--- a/src/test/crypto_tests.cpp
+++ b/src/test/crypto_tests.cpp
@@ -574,10 +574,10 @@ BOOST_AUTO_TEST_CASE(hkdf_hmac_sha256_l32_tests)
 {
     // Use rfc5869 test vectors but truncated to 32 bytes (our implementation only support length 32)
     TestHKDF_SHA256_32(
-                /*IKM=*/"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b",
-                /*salt=*/"000102030405060708090a0b0c",
-                /*info=*/"f0f1f2f3f4f5f6f7f8f9",
-                /* expected OKM */ "3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf");
+                /*ikm_hex=*/"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b",
+                /*salt_hex=*/"000102030405060708090a0b0c",
+                /*info_hex=*/"f0f1f2f3f4f5f6f7f8f9",
+                /*okm_check_hex=*/"3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf");
     TestHKDF_SHA256_32(
                 "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f",
                 "606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f909192939495969798999a9b9c9d9e9fa0a1a2a3a4a5a6a7a8a9aaabacadaeaf",
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index b583ddb07..4f2ccb6eb 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -56,7 +56,7 @@ void Seed(FastRandomContext& ctx);
 static inline void SeedInsecureRand(SeedRand seed = SeedRand::SEED)
 {
     if (seed == SeedRand::ZEROS) {
-        g_insecure_rand_ctx = FastRandomContext(/*deterministic=*/true);
+        g_insecure_rand_ctx = FastRandomContext(/*fDeterministic=*/true);
     } else {
         Seed(g_insecure_rand_ctx);
     }

The diff indicates only the explicit name changes and not any functional difference. The new names are the names of the variable in the function definition.

I agree with these changes. Thanks for catching and correcting this, @MarcoFalke!

rajarshimaitra

ACK fa00447

Manually tested the scripted diff to verify the produce the same changes.

Though I am getting few errors in the full test like this

$ ( cd ./src/ && run-clang-tidy  -j $(nproc) )
3 warnings and 2 errors generated.
Error while processing /home/raj/github-repo/bitcoin/src/test/bswap_tests.cpp.
Suppressed 3 warnings (3 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).
clang-tidy-12 --use-color -p=/home/raj/github-repo/bitcoin /home/raj/github-repo/bitcoin/src/validation.cpp
error: unknown argument: '-fstack-reuse=none' [clang-diagnostic-error]
error: unsupported option '-fno-extended-identifiers' [clang-diagnostic-error]

And also quite a lot of warnings.

Am I doing clang-tidy wrong?

maflcko · 2021-11-21T07:36:04Z

error: unknown argument: '-fstack-reuse=none' [clang-diagnostic-error]

Did you run configure with clang and cleared the compile_commands.json?

rm compile_commands.json
./autogen.sh && ./configure CC=clang CXX=clang++

jonatack

ACK fa00447

fanquake

ACK fa00447

… C++ named arguments (tests only) 6443c75 scripted-diff: Use clang-tidy syntax for C++ named arguments (MarcoFalke) 8859440 doc: Use clang-tidy comments in crypto_tests (MarcoFalke) Pull request description: Incorrect named args are source of bugs, like #22979. To allow them being checked by `clang-tidy`, use a format it can understand. ACKs for top commit: shaavan: ACK 6443c75 rajarshimaitra: ACK bitcoin/bitcoin@6443c75 jonatack: ACK 6443c75 fanquake: ACK 6443c75 Tree-SHA512: 4d23a8363da81dfea21a4cd8516ab5e0dc70119e4d503f3f240f38573218b2c2e84083b97e956c62942d78b2f17490f8b3b2e8077d257644fda1d901e2b80507

maflcko force-pushed the 2111-testClangTidy branch from bff3f48 to fabdf31 Compare November 18, 2021 20:03

DrahtBot added Refactoring Wallet labels Nov 18, 2021

maflcko force-pushed the 2111-testClangTidy branch from fabdf31 to fa8110a Compare November 19, 2021 11:35

MarcoFalke added 2 commits November 19, 2021 12:40

doc: Use clang-tidy comments in crypto_tests

fae13c3

Also, fix argument name for FastRandomContext.

scripted-diff: Use clang-tidy syntax for C++ named arguments

fa00447

-BEGIN VERIFY SCRIPT- perl -0777 -pi -e 's:((\(|\{|,)(\n| )*)\/\* ?([^=* ]+) ?\*\/ ?:\1/*\4=*/:g' $( git ls-files ./src/test ./src/wallet/test ) -END VERIFY SCRIPT-

maflcko force-pushed the 2111-testClangTidy branch from fa8110a to fa00447 Compare November 19, 2021 11:49

DrahtBot mentioned this pull request Nov 19, 2021

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Closed

shaavan approved these changes Nov 19, 2021

View reviewed changes

This was referenced Nov 19, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

net/net processing: Move tx inventory into net_processing #21160

Merged

rajarshimaitra reviewed Nov 20, 2021

View reviewed changes

jonatack reviewed Nov 22, 2021

View reviewed changes

DrahtBot mentioned this pull request Nov 23, 2021

fuzz: Rework FillNode #23575

Merged

jonatack mentioned this pull request Nov 23, 2021

refactor: Make CWalletTx sync state type-safe #21206

Merged

DrahtBot mentioned this pull request Nov 27, 2021

Use Sock in CNode #23604

Merged

fanquake approved these changes Dec 1, 2021

View reviewed changes

fanquake merged commit 205877e into bitcoin:master Dec 1, 2021

maflcko deleted the 2111-testClangTidy branch December 1, 2021 10:46

JeremyRubin mentioned this pull request Dec 15, 2021

Code style PRs after v0.18 branch split #15465

Closed

bitcoin locked and limited conversation to collaborators Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

Uh oh!

maflcko commented Nov 18, 2021

Uh oh!

DrahtBot commented Nov 18, 2021 •

edited

Loading

Uh oh!

katesalazar commented Nov 18, 2021

Uh oh!

maflcko commented Nov 19, 2021

Uh oh!

shaavan left a comment

Uh oh!

rajarshimaitra left a comment

Uh oh!

maflcko commented Nov 21, 2021 •

edited

Loading

Uh oh!

jonatack left a comment

Uh oh!

fanquake left a comment

Uh oh!

Uh oh!

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

Uh oh!

Conversation

maflcko commented Nov 18, 2021

Uh oh!

DrahtBot commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

katesalazar commented Nov 18, 2021

Uh oh!

maflcko commented Nov 19, 2021

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Nov 18, 2021 •

edited

Loading

maflcko commented Nov 21, 2021 •

edited

Loading