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

MarcoFalke commented at 7:24 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 commented at 7:25 pm on November 18, 2021: member

Let’s see how many conflicts this has :sweat_smile:

DrahtBot commented at 7:30 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:

#24165 (p2p: extend inbound eviction protection by network to CJDNS peers by jonatack)
#23604 (Use Sock in CNode by vasild)
#22910 (net: Encapsulate asmap in NetGroupManager by jnewbery)
#17167 (Allow whitelisting outgoing connections by luke-jr)

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.

MarcoFalke renamed this:
~~scripted-diff: Use clang-tidy syntax for C++ named arguments~~
scripted-diff: Use clang-tidy syntax for C++ named arguments [WIP; TOO MANY CONFLICTS]
on Nov 18, 2021

MarcoFalke marked this as a draft on Nov 18, 2021

katesalazar commented at 8:30 pm on November 18, 2021: contributor

Concept ACK.

DrahtBot added the label Consensus on Nov 18, 2021

DrahtBot added the label GUI on Nov 18, 2021

DrahtBot added the label Mempool on Nov 18, 2021

DrahtBot added the label Mining on Nov 18, 2021

DrahtBot added the label P2P on Nov 18, 2021

DrahtBot added the label Refactoring on Nov 18, 2021

DrahtBot added the label RPC/REST/ZMQ on Nov 18, 2021

DrahtBot added the label UTXO Db and Indexes on Nov 18, 2021

DrahtBot added the label Validation on Nov 18, 2021

DrahtBot added the label Wallet on Nov 18, 2021

MarcoFalke force-pushed on Dec 1, 2021

MarcoFalke force-pushed on Dec 3, 2021

in doc/developer-notes.md:170 in 9a5e148ab2 outdated

165+
166+```sh
167+apt install clang-tidy bear clang
168+```
169+
170+Then, pass clang as compiler to configure, and use bear to produce the `compile_commands.json`:

jonatack commented at 1:44 pm on December 5, 2021:

s/as compiler to configure/as the compiler to configure/

in doc/developer-notes.md:149 in 9a5e148ab2 outdated

141@@ -142,6 +142,51 @@ Coding Style (Python)
142 
143 Refer to [/test/functional/README.md#style-guidelines](/test/functional/README.md#style-guidelines).
144 
145+Coding Style (named arguments)
146+------------------------------
147+
148+When passing named arguments, use a format that clang-tidy understands. The
149+argument names can otherwise not be verified by clang-tidy.

jonatack commented at 1:44 pm on December 5, 2021:

s/can ... not/cannot/

suggested simplification: “that clang-tidy understands to enable it to verify argument names.”

in doc/developer-notes.md:145 in 9a5e148ab2 outdated

141@@ -142,6 +142,51 @@ Coding Style (Python)
142 
143 Refer to [/test/functional/README.md#style-guidelines](/test/functional/README.md#style-guidelines).
144 
145+Coding Style (named arguments)

jonatack commented at 1:46 pm on December 5, 2021:

Maybe place this in the Coding Style (C++) section just above or s/Coding Style (named arguments)/Coding Style (C++ named arguments)

jonatack commented at 2:04 pm on December 5, 2021: member

ACK 5ae180c542dbb43354ba944fdcbaa5ef7bc92f51 rebased to master, verified the scripted diff with test/lint/commit-script-check.sh ea989de..2a4582d and ran ( cd src/ && run-clang-tidy-14 -j $(nproc) ) on debian 5.15.5-1 (2021-11-26)

meshcollider removed the label GUI on Dec 8, 2021

meshcollider removed the label Wallet on Dec 8, 2021

meshcollider removed the label UTXO Db and Indexes on Dec 8, 2021

meshcollider removed the label RPC/REST/ZMQ on Dec 8, 2021

meshcollider removed the label P2P on Dec 8, 2021

meshcollider removed the label Mining on Dec 8, 2021

meshcollider removed the label Validation on Dec 8, 2021

meshcollider removed the label Mempool on Dec 8, 2021

meshcollider removed the label Consensus on Dec 8, 2021

MarcoFalke force-pushed on Dec 10, 2021

MarcoFalke renamed this:
~~scripted-diff: Use clang-tidy syntax for C++ named arguments [WIP; TOO MANY CONFLICTS]~~
scripted-diff: Use clang-tidy syntax for C++ named arguments
on Dec 10, 2021

in src/core_write.cpp:218 in c41736ab82 outdated

214@@ -215,7 +215,7 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
215 
216                 case TxVerbosity::SHOW_DETAILS_AND_PREVOUT:
217                     UniValue o_script_pub_key(UniValue::VOBJ);
218-                    ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /* includeHex */ true);
219+                    ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /*includeHex=*/true);

fanquake commented at 3:22 am on December 11, 2021:

0                    ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /*include_hex=*/true);

 0clang-tidy-12 --use-color -p=/home/ubuntu/bitcoin /home/ubuntu/bitcoin/src/core_write.cpp
 1/home/ubuntu/bitcoin/src/core_write.cpp:218:83: error: argument name 'includeHex' in comment does not match parameter name 'include_hex' [bugprone-argument-comment,-warnings-as-errors]
 2                    ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /*includeHex=*/true);
 3                                                                                  ^~~~~~~~~~~~~~~
 4                                                                                  /*include_hex=*/
 5./core_io.h:56:74: note: 'include_hex' declared here
 6void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);
 7                                                                         ^
 8/home/ubuntu/bitcoin/src/core_write.cpp:150:6: note: actual callee ('ScriptPubKeyToUniv') is declared here
 9void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address)
10     ^
111 warning generated.

MarcoFalke commented at 3:05 pm on January 4, 2022:

Already fixed in master

fanquake commented at 3:35 am on December 11, 2021: member

Running against c41736ab82cfb3b7ce8487c038eceb3617b82eb4:

 0clang-tidy-12 --use-color -p=/home/ubuntu/bitcoin /home/ubuntu/bitcoin/src/wallet/test/wallet_tests.cpp
 1wallet/test/wallet_tests.cpp:333:155: error: argument name 'position_in_block' in comment does not match parameter name 'index' [bugprone-argument-comment,-warnings-as-errors]
 2    CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/0}};
 3                                                                                                                                                          ^
 4./wallet/transaction.h:28:74: note: 'index' declared here
 5    explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
 6                                                                         ^
 7wallet/test/wallet_tests.cpp:367:56: error: argument name 'position_in_block' in comment does not match parameter name 'index' [bugprone-argument-comment,-warnings-as-errors]
 8        state = TxStateConfirmed{hash, block->nHeight, /*position_in_block=*/0};
 9                                                       ^
10./wallet/transaction.h:28:74: note: 'index' declared here
11    explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
12                                                                         ^
13wallet/test/wallet_tests.cpp:534:142: error: argument name 'position_in_block' in comment does not match parameter name 'index' [bugprone-argument-comment,-warnings-as-errors]
14        it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/1};
15                                                                                                                                             ^
16./wallet/transaction.h:28:74: note: 'index' declared here
17    explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
18                                                                         ^
19/home/ubuntu/bitcoin/src/wallet/test/wallet_tests.cpp:825:109: error: argument name 'update' in comment does not match parameter name 'fUpdate' [bugprone-argument-comment,-warnings-as-errors]
20        wallet->ScanForWalletTransactions(m_node.chain->getBlockHash(0), 0, /* max height= */ {}, reserver, /* update= */ true);
21                                                                                                            ^~~~~~~~~~~~~
22                                                                                                            /* fUpdate= */
23./wallet/wallet.h:534:162: note: 'fUpdate' declared here
24    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);

MarcoFalke force-pushed on Dec 13, 2021

MarcoFalke force-pushed on Dec 15, 2021

MarcoFalke force-pushed on Jan 4, 2022

MarcoFalke force-pushed on Jan 17, 2022

MarcoFalke commented at 8:26 am on January 17, 2022: member

Running against c41736a:

Pretty sure those are bugs in current master and have nothing to do with this pull? Maybe create a separate issues or pull to fix those?

DrahtBot added the label Needs rebase on Jan 25, 2022

MarcoFalke force-pushed on Jan 25, 2022

DrahtBot removed the label Needs rebase on Jan 25, 2022

DrahtBot commented at 8:59 am on February 4, 2022: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Feb 4, 2022

MarcoFalke force-pushed on Feb 4, 2022

doc: Document clang-tidy in dev notes 95c382507e

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

-BEGIN VERIFY SCRIPT-
 sed -i --regexp-extended -e 's_(\(|\{|, ?)\/\* ?([^=* ]+) ?\*\/ ?_\1/*\2=*/_g' $( git grep -l --extended-regexp ', ?\/\* ?[^=* ]+ ?\*\/' ./src )
 # perl -0777 -pi -e 's:((\(|\{|,)(\n| )*)\/\* ?([^=* ]+) ?\*\/ ?:\1/*\4=*/:g' $( git ls-files ./src/ )
-END VERIFY SCRIPT-

63971b4d01

MarcoFalke force-pushed on Feb 4, 2022

MarcoFalke commented at 11:00 am on February 4, 2022: member

Closing as up for grabs

MarcoFalke closed this on Feb 4, 2022

MarcoFalke deleted the branch on Feb 4, 2022

fanquake deleted a comment on Feb 4, 2022

fanquake commented at 5:07 pm on March 24, 2022: member

Closing as up for grabs

Picked up in #24661.

MarcoFalke referenced this in commit 0da559e02e on Apr 4, 2022

DrahtBot locked this on Mar 24, 2023

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

Conflicts