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

MarcoFalke commented at 8:00 pm on November 18, 2021: member

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

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

MarcoFalke force-pushed on Nov 18, 2021

DrahtBot commented at 8:05 pm on November 18, 2021: member

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 commented at 8:29 pm on November 18, 2021: contributor

Concept ACK, thanks.

DrahtBot added the label Refactoring on Nov 18, 2021

DrahtBot added the label Wallet on Nov 18, 2021

MarcoFalke force-pushed on Nov 19, 2021

doc: Use clang-tidy comments in crypto_tests

Also, fix argument name for FastRandomContext.

fae13c3989

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

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

fa00447442

MarcoFalke force-pushed on Nov 19, 2021

MarcoFalke commented at 12:34 pm on November 19, 2021: member

Setup clang-tidy:

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

Full test:

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

Diff test:

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

shaavan approved

shaavan commented at 1:05 pm on November 19, 2021: contributor

ACK fa00447442f22a24e5ca5fc538d0bf7bef575544

I independently recreated the changes done in this PR by:

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

0perl -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:

 0--- a/src/test/crypto_tests.cpp
 1+++ b/src/test/crypto_tests.cpp
 2@@ -574,10 +574,10 @@ BOOST_AUTO_TEST_CASE(hkdf_hmac_sha256_l32_tests)
 3 {
 4     // Use rfc5869 test vectors but truncated to 32 bytes (our implementation only support length 32)
 5     TestHKDF_SHA256_32(
 6-                /*IKM=*/"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b",
 7-                /*salt=*/"000102030405060708090a0b0c",
 8-                /*info=*/"f0f1f2f3f4f5f6f7f8f9",
 9-                /* expected OKM */ "3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf");
10+                /*ikm_hex=*/"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b",
11+                /*salt_hex=*/"000102030405060708090a0b0c",
12+                /*info_hex=*/"f0f1f2f3f4f5f6f7f8f9",
13+                /*okm_check_hex=*/"3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf");
14     TestHKDF_SHA256_32(
15                 "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f",
16                 "606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f909192939495969798999a9b9c9d9e9fa0a1a2a3a4a5a6a7a8a9aaabacadaeaf",
17diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
18index b583ddb07..4f2ccb6eb 100644
19--- a/src/test/util/setup_common.h
20+++ b/src/test/util/setup_common.h
21@@ -56,7 +56,7 @@ void Seed(FastRandomContext& ctx);
22 static inline void SeedInsecureRand(SeedRand seed = SeedRand::SEED)
23 {
24     if (seed == SeedRand::ZEROS) {
25-        g_insecure_rand_ctx = FastRandomContext(/*deterministic=*/true);
26+        g_insecure_rand_ctx = FastRandomContext(/*fDeterministic=*/true);
27     } else {
28         Seed(g_insecure_rand_ctx);
29     }

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 commented at 3:39 pm on November 20, 2021: contributor

ACK https://github.com/bitcoin/bitcoin/pull/23546/commits/fa00447442f22a24e5ca5fc538d0bf7bef575544

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

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

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

And also quite a lot of warnings.

Am I doing clang-tidy wrong?

MarcoFalke commented at 7:36 am on November 21, 2021: member

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

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

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

jonatack commented at 12:14 pm on November 22, 2021: member

ACK fa00447442f22a24e5ca5fc538d0bf7bef575544

fanquake approved

fanquake commented at 10:38 am on December 1, 2021: member

ACK fa00447442f22a24e5ca5fc538d0bf7bef575544

fanquake merged this on Dec 1, 2021

fanquake closed this on Dec 1, 2021

MarcoFalke deleted the branch on Dec 1, 2021

RandyMcMillan referenced this in commit ad1fde0b39 on Dec 23, 2021

DrahtBot locked this on Dec 1, 2022

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

Conflicts