doc: Fix incorrect C++ named args by maflcko · Pull Request #22981 · bitcoin/bitcoin

maflcko · 2021-09-15T12:20:35Z

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.

maflcko · 2021-09-15T12:23:29Z

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 · 2021-09-15T18:36:36Z

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

DrahtBot · 2021-09-15T21:56:23Z

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.

fanquake

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.

src/qt/test/test_main.cpp

maflcko · 2021-09-16T06:43:56Z

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.

src/bench/rpc_mempool.cpp

src/psbt.cpp

laanwj · 2021-09-21T11:41:53Z

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

practicalswift · 2021-10-20T08:33:47Z

Concept ACK

jnewbery · 2021-11-05T07:29:06Z

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

maflcko · 2021-11-05T09:16:23Z

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

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 ?

maflcko · 2021-11-11T17:46:18Z

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 · 2021-11-13T18:16:09Z

Concept ACK.

fanquake · 2021-11-16T05:19:55Z

/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

maflcko · 2021-11-16T12:41:50Z

Good catch. Fixed in #23525

maflcko · 2021-11-17T13:47:37Z

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

fanquake

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

fac4947 doc: Fix incorrect C++ named args (MarcoFalke) Pull request description: Incorrect named args are source of bugs, like bitcoin#22979. Fix that by correcting them and adjust the format, so that clang-tidy can check it. ACKs for top commit: fanquake: ACK fac4947 - `run-clang-tidy` works for me now. Tree-SHA512: 2694e17a1586394baf30bbc479a913e4bad361221e8470b8739caf30aacea736befc73820f3fe56f6207d9f5d969323278d43a647f58c3497e8e44cad79f8934

maflcko added the Refactoring label Sep 15, 2021

fanquake reviewed Sep 16, 2021

View reviewed changes

src/qt/test/test_main.cpp Outdated Show resolved Hide resolved

This was referenced Sep 16, 2021

Package Mempool Submission with Package Fee-Bumping #22290

Closed

refactor: Make CWalletTx sync state type-safe #21206

Merged

laanwj reviewed Sep 21, 2021

View reviewed changes

src/bench/rpc_mempool.cpp Outdated Show resolved Hide resolved

laanwj reviewed Sep 21, 2021

View reviewed changes

src/psbt.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2109-docNamedArgs branch 3 times, most recently from fa47e7e to fab1408 Compare September 27, 2021 10:42

DrahtBot mentioned this pull request Sep 29, 2021

Make all networking code mockable #21878

Closed

12 tasks

DrahtBot mentioned this pull request Oct 15, 2021

tests: remove usage of LegacyScriptPubKeyMan from some wallet tests #23288

Merged

bitcoin deleted a comment from MooCricket Oct 18, 2021

DrahtBot added the Needs rebase label Oct 19, 2021

maflcko force-pushed the 2109-docNamedArgs branch from fab1408 to fa41833 Compare October 19, 2021 14:47

DrahtBot removed the Needs rebase label Oct 19, 2021

DrahtBot mentioned this pull request Oct 22, 2021

doc: Fix CWalletTx::Confirmation doc #23332

Merged

DrahtBot added the Needs rebase label Oct 22, 2021

maflcko force-pushed the 2109-docNamedArgs branch from fa41833 to fadf4db Compare October 26, 2021 13:03

DrahtBot removed the Needs rebase label Oct 26, 2021

This was referenced Oct 26, 2021

test: Extend stale_tip_peer_management test #23352

Closed

test: Extend stale tip test #18470

Closed

DrahtBot mentioned this pull request Oct 28, 2021

validation/refactor: refactoring for package submission #23381

Merged

maflcko force-pushed the 2109-docNamedArgs branch from fadf4db to fa7efa0 Compare October 29, 2021 09:02

DrahtBot mentioned this pull request Nov 3, 2021

refactor: Remove char serialize #22167

Closed

jnewbery mentioned this pull request Nov 5, 2021

refactor: AcceptToMemoryPool #23437

Merged

lsilva01 approved these changes Nov 5, 2021

View reviewed changes

DrahtBot added the Needs rebase label Nov 9, 2021

maflcko force-pushed the 2109-docNamedArgs branch from fa7efa0 to faa5a80 Compare November 9, 2021 17:27

DrahtBot removed the Needs rebase label Nov 9, 2021

DrahtBot mentioned this pull request Nov 13, 2021

multiprocess: Add interfaces::Node::broadCastTransaction method #23499

Merged

DrahtBot added the Needs rebase label Nov 16, 2021

maflcko force-pushed the 2109-docNamedArgs branch from faa5a80 to fa83737 Compare November 17, 2021 08:22

doc: Fix incorrect C++ named args

fac4947

maflcko force-pushed the 2109-docNamedArgs branch from fa83737 to fac4947 Compare November 17, 2021 08:25

DrahtBot removed the Needs rebase label Nov 17, 2021

fanquake approved these changes Nov 18, 2021

View reviewed changes

fanquake merged commit 606e306 into bitcoin:master Nov 18, 2021

maflcko deleted the 2109-docNamedArgs branch November 18, 2021 18:21

bitcoin locked and limited conversation to collaborators Nov 18, 2022

Conversation

maflcko commented Sep 15, 2021

Uh oh!

maflcko commented Sep 15, 2021

Uh oh!

laanwj commented Sep 15, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

laanwj commented Sep 21, 2021

Uh oh!

practicalswift commented Oct 20, 2021

Uh oh!

jnewbery commented Nov 5, 2021

Uh oh!

maflcko commented Nov 5, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 11, 2021

Uh oh!

katesalazar commented Nov 13, 2021

Uh oh!

fanquake commented Nov 16, 2021

Uh oh!

maflcko commented Nov 16, 2021

Uh oh!

maflcko commented Nov 17, 2021

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Sep 15, 2021 •

edited

Loading