doc: Fix incorrect C++ named args

MarcoFalke commented at 12:20 pm on September 15, 2021: member

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

Fix that by correcting them and adjust the format, so that clang-tidy can check it.

MarcoFalke added the label Refactoring on Sep 15, 2021

MarcoFalke commented at 12:23 pm on September 15, 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) )

laanwj commented at 6:36 pm on September 15, 2021: member

Concept ACK, being able to check these automatically with a tool is a good idea.

DrahtBot commented at 9:56 pm on September 15, 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:

#23499 (multiprocess: Add interfaces::Node::broadCastTransaction method by ryanofsky)
#23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
#22793 (Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors by achow101)
#21878 (Make all networking code mockable by vasild)
#18470 (net: Make stale tip check time type-safe, extend test by MarcoFalke)

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.

in src/qt/test/test_main.cpp:72 in fab6b32322 outdated

69@@ -70,7 +70,7 @@ int main(int argc, char* argv[])
70     #if defined(WIN32)
71         if (getenv("QT_QPA_PLATFORM") == nullptr) _putenv_s("QT_QPA_PLATFORM", "minimal");
72     #else
73-        setenv("QT_QPA_PLATFORM", "minimal", /* overwrite */ 0);
74+        setenv("QT_QPA_PLATFORM", "minimal", 0 /* overwrite */);

fanquake commented at 3:53 am on September 16, 2021:

0        setenv("QT_QPA_PLATFORM", "minimal", /* overwrite */0);

I assume this isn’t updated to /*overwrite=*/ because that will fail the check:

0/bitcoin/src/qt/test/test_main.cpp:73:46: error: argument name 'overwrite' in comment does not match parameter name '__replace' [bugprone-argument-comment,-warnings-as-errors]
1        setenv("QT_QPA_PLATFORM", "minimal", /*overwrite=*/0);
2                                             ^
3/usr/include/stdlib.h:653:65: note: '__replace' declared here
4extern int setenv (const char *__name, const char *__value, int __replace)

If that’s the case, why move the argument to before the comment, clang-tidy still works when it’s trailing.

MarcoFalke commented at 6:42 am on September 16, 2021:

I think for consistency named args that are not enforced should be trailing. If it was kept in the beginning, it could confuse devs and trick them into adding an =, which again would fail clang-tidy. See also the comment in src/test/fuzz/tx_pool.cpp, which I moved to the end.

laanwj commented at 11:38 am on September 21, 2021:

maybe /* don't overwrite */ ? if it doesn’t look like an argument name, any confusion is gone

MarcoFalke commented at 11:42 am on September 21, 2021:

It is the correct argument name according to man, but the headers shipped on the system may pick a different name (implementation detail).

0$ man setenv | head -9
1SETENV(3)             Linux Programmer's Manual            SETENV(3)
2
3NAME
4       setenv - change or add an environment variable
5
6SYNOPSIS
7       #include <stdlib.h>
8
9       int setenv(const char *name, const char *value, int overwrite);

fanquake commented at 4:03 am on September 16, 2021: member

Concept ACK - Tested using the steps below. If we are going to start making more of these changes we should be integrating this into the CI at some point soon.

 0docker run -it --rm ubuntu:hirsute
 1
 2apt update && apt upgrade
 3
 4apt install git build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 clang-12 clang-tidy-12 bear libevent-dev libboost-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev libdb-dev libdb++-dev
 5
 6clang-12 --version
 7Ubuntu clang version 12.0.0-3ubuntu1~21.04.2
 8Target: x86_64-pc-linux-gnu
 9Thread model: posix
10InstalledDir: /usr/bin
11
12clang-tidy-12 --version
13LLVM (http://llvm.org/):
14  LLVM version 12.0.0
15  
16  Optimized build.
17  Default target: x86_64-pc-linux-gnu
18  Host CPU: skylake-avx512
19
20bear --version
21bear 3.0.8
22
23git clone https://github.com/bitcoin/bitcoin
24cd bitcoin
25./autogen
26./configure --with-incompatible-bdb CC=clang-12 CXX=clang++-12
27make clean && bear -- make -j $(nproc)
28
29( cd ./src/ && run-clang-tidy-12  -j $(nproc) )
30Enabled checks:
31    bugprone-argument-comment
32
33clang-tidy-12 --use-color -p=/bitcoin /bitcoin/src/blockfilter.cpp
34clang-tidy-12 --use-color -p=/bitcoin /bitcoin/src/chain.cpp
35clang-tidy-12 --use-color -p=/bitcoin /bitcoin/src/banman.cpp
36<snipp>
37
38git diff | ( cd ./src/ && clang-tidy-diff-12.py -p2 -j $(nproc) )
39No relevant changes found.

MarcoFalke commented at 6:43 am on September 16, 2021: member

we should be integrating this into the CI at some point soon.

Happy to review a pull doing this. Though, I think it is also acceptable to run it only once before every major release.

in src/bench/rpc_mempool.cpp:15 in fab6b32322 outdated

11@@ -12,7 +12,7 @@
12 static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
13 {
14     LockPoints lp;
15-    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /* time */ 0, /* height */ 1, /* spendsCoinbase */ false, /* sigOpCost */ 4, lp));
16+    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*_nTime=*/0, /*_entryHeight=*/1, /*spendsCoinbase=*/false, /*nSigOpsCost=*/4, lp));

laanwj commented at 11:36 am on September 21, 2021:

It’s unfortunate in these cases that these argument names are so ugly (e.g. _nTime doesn’t just have an underscore, but also the long deprecated syntax)

MarcoFalke commented at 11:37 am on September 21, 2021:

Happy to review a follow-up to fixup the name of the args, or should I do it here?

laanwj commented at 11:40 am on September 21, 2021:

I don’t know. It does make me reconsider whether automatically checking this is a good idea.

MarcoFalke commented at 11:45 am on September 21, 2021:

Not checking them makes it harder to catch bugs, like #22979. So I think this is overall a good idea.

MarcoFalke commented at 12:14 pm on September 21, 2021:

Fixed in #23054

in src/psbt.cpp:226 in fab6b32322 outdated

222@@ -223,7 +223,7 @@ void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransactio
223     // Construct a would-be spend of this output, to update sigdata with.
224     // Note that ProduceSignature is used to fill in metadata (not actual signatures),
225     // so provider does not need to provide any private keys (it can be a HidingSigningProvider).
226-    MutableTransactionSignatureCreator creator(&tx, /* index */ 0, out.nValue, SIGHASH_ALL);
227+    MutableTransactionSignatureCreator creator(&tx, /*nInIn=*/0, out.nValue, SIGHASH_ALL);

laanwj commented at 11:36 am on September 21, 2021:

Same here. I’m not sure this is an improvement in readability. What is a nInIn? Might as well strip the comment out.

MarcoFalke commented at 11:39 am on September 21, 2021:

nInIn is used as comment elsewhere, so this is a pre-existing issue. I think every literal numeric type passed to a function should have a name.

I can offer to rename nInIn here or in a follow-up.

MarcoFalke commented at 2:43 pm on September 21, 2021:

Done here in a new commit

laanwj commented at 11:41 am on September 21, 2021: member

I think we should first make sure our argument names make sense, then do this.

MarcoFalke force-pushed on Sep 21, 2021

MarcoFalke force-pushed on Sep 24, 2021

MarcoFalke force-pushed on Sep 27, 2021

fanquake deleted a comment on Oct 18, 2021

DrahtBot added the label Needs rebase on Oct 19, 2021

MarcoFalke force-pushed on Oct 19, 2021

DrahtBot removed the label Needs rebase on Oct 19, 2021

practicalswift commented at 8:33 am on October 20, 2021: contributor

Concept ACK

DrahtBot added the label Needs rebase on Oct 22, 2021

MarcoFalke force-pushed on Oct 26, 2021

DrahtBot removed the label Needs rebase on Oct 26, 2021

MarcoFalke force-pushed on Oct 29, 2021

jnewbery commented at 7:29 am on November 5, 2021: member

Concept ACK. Should we document the required comment style somewhere in the developer docs?

MarcoFalke commented at 9:16 am on November 5, 2021: member

Happy to document the style in a follow-up. I have one follow-up myself that adjusts the style of existing args. The goal of this pull is to replace wrong names with correct names.

lsilva01 approved

lsilva01 commented at 8:23 pm on November 5, 2021: contributor

tACK fa7efa0

The commands described in #22981 (comment) ran successfully.

And after clang-tidy-10 returned without an error, I changed a commented named argument in a function, reran it, and the error was identified.

Should compile_commands.json be added to .gitignore ?

DrahtBot added the label Needs rebase on Nov 9, 2021

MarcoFalke force-pushed on Nov 9, 2021

DrahtBot removed the label Needs rebase on Nov 9, 2021

MarcoFalke commented at 5:46 pm on November 11, 2021: member

Should compile_commands.json be added to .gitignore ?

Yeah, will do on the next force push or the next follow-up pull, unless someone beats me to it.

katesalazar commented at 6:16 pm on November 13, 2021: contributor

Concept ACK.

fanquake commented at 5:19 am on November 16, 2021: member

0/usr/local/opt/llvm/bin/clang-tidy --use-color -p=/Users/michael/github/bitcoin-merge-tree /Users/michael/github/bitcoin-merge-tree/src/psbt.cpp
1psbt.cpp:226:53: error: argument name 'input_idx' in comment does not match parameter name 'nInIn' [bugprone-argument-comment,-warnings-as-errors]
2    MutableTransactionSignatureCreator creator(&tx, /*input_idx=*/0, out.nValue, SIGHASH_ALL);
3                                                    ^
4./script/sign.h:48:88: note: 'nInIn' declared here
5    MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn);
6                                                                                       ^
71 warning generated.
81 warning treated as error

DrahtBot added the label Needs rebase on Nov 16, 2021

MarcoFalke commented at 12:41 pm on November 16, 2021: member

Good catch. Fixed in https://github.com/bitcoin/bitcoin/pull/23525

MarcoFalke force-pushed on Nov 17, 2021

doc: Fix incorrect C++ named args fac49470ca

MarcoFalke force-pushed on Nov 17, 2021

DrahtBot removed the label Needs rebase on Nov 17, 2021

MarcoFalke commented at 1:47 pm on November 17, 2021: member

Rebased to get rid of already fixed instances. Only 29 left in this pull for now.

fanquake approved

fanquake commented at 0:17 am on November 18, 2021: member

ACK fac49470ca36ff944a613f4358386bf8e0967427 - run-clang-tidy works for me now.

fanquake merged this on Nov 18, 2021

fanquake closed this on Nov 18, 2021

MarcoFalke deleted the branch on Nov 18, 2021

sidhujag referenced this in commit b7fb4bf7b7 on Nov 19, 2021

sidhujag referenced this in commit 31add26bf4 on Nov 19, 2021

DrahtBot locked this on Nov 18, 2022

doc: Fix incorrect C++ named args #22981

Conflicts