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

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2109-docNamedArgs changing 32 files +126 −81
  1. 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.

  2. MarcoFalke commented at 7:25 PM on November 18, 2021: member

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

  3. DrahtBot commented at 7:30 PM on November 18, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  4. 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
  5. MarcoFalke marked this as a draft on Nov 18, 2021
  6. katesalazar commented at 8:30 PM on November 18, 2021: contributor

    Concept ACK.

  7. DrahtBot added the label Consensus on Nov 18, 2021
  8. DrahtBot added the label GUI on Nov 18, 2021
  9. DrahtBot added the label Mempool on Nov 18, 2021
  10. DrahtBot added the label Mining on Nov 18, 2021
  11. DrahtBot added the label P2P on Nov 18, 2021
  12. DrahtBot added the label Refactoring on Nov 18, 2021
  13. DrahtBot added the label RPC/REST/ZMQ on Nov 18, 2021
  14. DrahtBot added the label UTXO Db and Indexes on Nov 18, 2021
  15. DrahtBot added the label Validation on Nov 18, 2021
  16. DrahtBot added the label Wallet on Nov 18, 2021
  17. DrahtBot cross-referenced this on Nov 19, 2021 from issue Add getdeploymentinfo RPC by ajtowns
  18. DrahtBot cross-referenced this on Nov 19, 2021 from issue rpc: Only allow specific types to be P2(W)SH wrapped in decodescript by MarcoFalke
  19. DrahtBot cross-referenced this on Nov 19, 2021 from issue Remove CTxMemPool params from ATMP by lsilva01
  20. DrahtBot cross-referenced this on Nov 19, 2021 from issue refactor: AcceptToMemoryPool by lsilva01
  21. DrahtBot cross-referenced this on Nov 19, 2021 from issue Optimize coin selection by dropping BnB upper limit by S3RK
  22. DrahtBot cross-referenced this on Nov 19, 2021 from issue WIP: [RPC] allow fetching headers by pages by JeremyRubin
  23. DrahtBot cross-referenced this on Nov 19, 2021 from issue rpc: Add RPC help for getblock verbosity level 3 by kiminuo
  24. DrahtBot cross-referenced this on Nov 19, 2021 from issue init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths by dongcarl
  25. DrahtBot cross-referenced this on Nov 19, 2021 from issue wallet: allow psbtbumpfee to work with txs with external inputs by achow101
  26. DrahtBot cross-referenced this on Nov 19, 2021 from issue wallet: Allow users to specify input weights when funding a transaction by achow101
  27. DrahtBot cross-referenced this on Nov 19, 2021 from issue rpc, wallet: Add listaddresses RPC by lsilva01
  28. DrahtBot cross-referenced this on Nov 19, 2021 from issue [TESTS] Allow tx_invalid.json tests to include flag rules for if_unset: [A,B,C] then_unset: [D] by JeremyRubin
  29. DrahtBot cross-referenced this on Nov 19, 2021 from issue refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag by mjdietzx
  30. DrahtBot cross-referenced this on Nov 19, 2021 from issue Add allocator for node based containers by martinus
  31. DrahtBot cross-referenced this on Nov 19, 2021 from issue psbt: Taproot fields for PSBT by achow101
  32. DrahtBot cross-referenced this on Nov 19, 2021 from issue wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101
  33. DrahtBot cross-referenced this on Nov 19, 2021 from issue rpc: add period_start to version bits statistics by Sjors
  34. DrahtBot cross-referenced this on Nov 19, 2021 from issue Make all networking code mockable by vasild
  35. DrahtBot cross-referenced this on Nov 19, 2021 from issue Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin
  36. DrahtBot cross-referenced this on Nov 19, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by MarcoFalke
  37. DrahtBot cross-referenced this on Nov 19, 2021 from issue Implement BIP 370 PSBTv2 by achow101
  38. DrahtBot cross-referenced this on Nov 19, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
  39. DrahtBot cross-referenced this on Nov 19, 2021 from issue rpc, test: Improve getblockstats for unspendables by fjahr
  40. DrahtBot cross-referenced this on Nov 19, 2021 from issue Allow whitelisting outgoing connections by luke-jr
  41. DrahtBot cross-referenced this on Nov 19, 2021 from issue Let validateaddress locate error in Bech32 address by meshcollider
  42. hebasto cross-referenced this on Nov 21, 2021 from issue Monospaced output in Console on macOS by RandyMcMillan
  43. MarcoFalke force-pushed on Dec 1, 2021
  44. DrahtBot cross-referenced this on Dec 1, 2021 from issue refactor: Call type-solver earlier in decodescript by MarcoFalke
  45. DrahtBot cross-referenced this on Dec 2, 2021 from issue Use Sock in CNode by vasild
  46. DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider
  47. MarcoFalke cross-referenced this on Dec 3, 2021 from issue doc: Document optional RPC result fields by MarcoFalke
  48. MarcoFalke force-pushed on Dec 3, 2021
  49. DrahtBot cross-referenced this on Dec 4, 2021 from issue Split up rpcwallet by meshcollider
  50. 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/

  51. 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."

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

  53. jonatack commented at 2:04 PM on December 5, 2021: contributor

    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)

  54. meshcollider removed the label GUI on Dec 8, 2021
  55. meshcollider removed the label Wallet on Dec 8, 2021
  56. meshcollider removed the label UTXO Db and Indexes on Dec 8, 2021
  57. meshcollider removed the label RPC/REST/ZMQ on Dec 8, 2021
  58. meshcollider removed the label P2P on Dec 8, 2021
  59. meshcollider removed the label Mining on Dec 8, 2021
  60. meshcollider removed the label Validation on Dec 8, 2021
  61. meshcollider removed the label Mempool on Dec 8, 2021
  62. meshcollider removed the label Consensus on Dec 8, 2021
  63. MarcoFalke force-pushed on Dec 10, 2021
  64. 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
  65. 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:
                        ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /*include_hex=*/true);
    
    clang-tidy-12 --use-color -p=/home/ubuntu/bitcoin /home/ubuntu/bitcoin/src/core_write.cpp
    /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]
                        ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /*includeHex=*/true);
                                                                                      ^~~~~~~~~~~~~~~
                                                                                      /*include_hex=*/
    ./core_io.h:56:74: note: 'include_hex' declared here
    void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);
                                                                             ^
    /home/ubuntu/bitcoin/src/core_write.cpp:150:6: note: actual callee ('ScriptPubKeyToUniv') is declared here
    void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address)
         ^
    1 warning generated.
    

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

    Already fixed in master

  66. fanquake commented at 3:35 AM on December 11, 2021: member

    Running against c41736ab82cfb3b7ce8487c038eceb3617b82eb4:

    clang-tidy-12 --use-color -p=/home/ubuntu/bitcoin /home/ubuntu/bitcoin/src/wallet/test/wallet_tests.cpp
    wallet/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]
        CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/0}};
                                                                                                                                                              ^
    ./wallet/transaction.h:28:74: note: 'index' declared here
        explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
                                                                             ^
    wallet/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]
            state = TxStateConfirmed{hash, block->nHeight, /*position_in_block=*/0};
                                                           ^
    ./wallet/transaction.h:28:74: note: 'index' declared here
        explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
                                                                             ^
    wallet/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]
            it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/1};
                                                                                                                                                 ^
    ./wallet/transaction.h:28:74: note: 'index' declared here
        explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
                                                                             ^
    /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]
            wallet->ScanForWalletTransactions(m_node.chain->getBlockHash(0), 0, /* max height= */ {}, reserver, /* update= */ true);
                                                                                                                ^~~~~~~~~~~~~
                                                                                                                /* fUpdate= */
    ./wallet/wallet.h:534:162: note: 'fUpdate' declared here
        ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
    
  67. DrahtBot cross-referenced this on Dec 11, 2021 from issue validation, log: improve logging of ChainstateManager snapshot persistance by jonatack
  68. DrahtBot cross-referenced this on Dec 11, 2021 from issue refactor, test: refactor addrman_tried_collisions test to directly check for collisions by josibake
  69. MarcoFalke force-pushed on Dec 13, 2021
  70. MarcoFalke force-pushed on Dec 15, 2021
  71. DrahtBot cross-referenced this on Dec 23, 2021 from issue net: Encapsulate asmap in NetGroupManager by jnewbery
  72. jnewbery cross-referenced this on Jan 1, 2022 from issue validation: followups for de-duplication of packages by glozow
  73. MarcoFalke force-pushed on Jan 4, 2022
  74. DrahtBot cross-referenced this on Jan 6, 2022 from issue wallet: add config to prioritize a solution that doesn't create change in coin selection by brunoerg
  75. DrahtBot cross-referenced this on Jan 9, 2022 from issue doc: format markdown files by erenalyoruk
  76. MarcoFalke force-pushed on Jan 17, 2022
  77. 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?

  78. DrahtBot added the label Needs rebase on Jan 25, 2022
  79. MarcoFalke force-pushed on Jan 25, 2022
  80. DrahtBot removed the label Needs rebase on Jan 25, 2022
  81. DrahtBot cross-referenced this on Jan 26, 2022 from issue p2p: extend inbound eviction protection by network to CJDNS peers by jonatack
  82. DrahtBot commented at 8:59 AM on February 4, 2022: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  83. DrahtBot added the label Needs rebase on Feb 4, 2022
  84. MarcoFalke force-pushed on Feb 4, 2022
  85. doc: Document clang-tidy in dev notes 95c382507e
  86. 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
  87. MarcoFalke force-pushed on Feb 4, 2022
  88. MarcoFalke commented at 11:00 AM on February 4, 2022: member

    Closing as up for grabs

  89. MarcoFalke closed this on Feb 4, 2022

  90. MarcoFalke deleted the branch on Feb 4, 2022
  91. bitcoin deleted a comment on Feb 4, 2022
  92. glozow cross-referenced this on Mar 10, 2022 from issue wallet: generate random change target for each tx for better privacy by glozow
  93. fanquake cross-referenced this on Mar 24, 2022 from issue refactor: Use clang-tidy syntax for C++ named arguments by fanquake
  94. fanquake commented at 5:07 PM on March 24, 2022: member

    Closing as up for grabs

    Picked up in #24661.

  95. MarcoFalke referenced this in commit 0da559e02e on Apr 4, 2022
  96. bitcoin locked this on Mar 24, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC