doc: Fix incorrect C++ named args #22981

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:

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) )

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:

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

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

/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]
        setenv("QT_QPA_PLATFORM", "minimal", /*overwrite=*/0);
                                             ^
/usr/include/stdlib.h:653:65: note: '__replace' declared here
extern 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).

$ man setenv | head -9
SETENV(3)             Linux Programmer's Manual            SETENV(3)

NAME
       setenv - change or add an environment variable

SYNOPSIS
       #include <stdlib.h>

       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.

docker run -it --rm ubuntu:hirsute

apt update && apt upgrade

apt 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

clang-12 --version
Ubuntu clang version 12.0.0-3ubuntu1~21.04.2
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

clang-tidy-12 --version
LLVM (http://llvm.org/):
  LLVM version 12.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: skylake-avx512

bear --version
bear 3.0.8

git clone https://github.com/bitcoin/bitcoin
cd bitcoin
./autogen
./configure --with-incompatible-bdb CC=clang-12 CXX=clang++-12
make clean && bear -- make -j $(nproc)

( cd ./src/ && run-clang-tidy-12  -j $(nproc) )
Enabled checks:
    bugprone-argument-comment

clang-tidy-12 --use-color -p=/bitcoin /bitcoin/src/blockfilter.cpp
clang-tidy-12 --use-color -p=/bitcoin /bitcoin/src/chain.cpp
clang-tidy-12 --use-color -p=/bitcoin /bitcoin/src/banman.cpp
<snipp>

git diff | ( cd ./src/ && clang-tidy-diff-12.py -p2 -j $(nproc) )
No 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

/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
psbt.cpp:226:53: error: argument name 'input_idx' in comment does not match parameter name 'nInIn' [bugprone-argument-comment,-warnings-as-errors]
    MutableTransactionSignatureCreator creator(&tx, /*input_idx=*/0, out.nValue, SIGHASH_ALL);
                                                    ^
./script/sign.h:48:88: note: 'nInIn' declared here
    MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn);
                                                                                       ^
1 warning generated.
1 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 12: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