clang-tidy: fixup named argument comments #26238

fanquake commented at 7:27 PM on October 3, 2022: member

Fix comments so they are checked/consistent. Fix incorrect comments.

in src/wallet/rpc/backup.cpp:276 in 7c59fb216a outdated

 272 | @@ -273,19 +273,19 @@ RPCHelpMan importaddress()
 273 |  
 274 |              pwallet->MarkDirty();
 275 |  
 276 | -            pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, false /* have_solving_data */, true /* apply_label */, 1 /* timestamp */);
 277 | +            pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, /*have_solving_data=*/false, /*apply_label=*/false, /*timestamp=*/1);

mzumsande commented at 10:02 PM on October 3, 2022:

apply_label is changed from true to false, so the tests fail.

DrahtBot commented at 11:35 PM on October 3, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Concept ACK	MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/650 (Add Import to Wallet GUI by KolbyML)
#25979 ([WIP] wallet: standardize change output detection process by furszy)
#25934 (wallet, rpc: add label to listsinceblock by brunoerg)
#25806 (wallet: group outputs only once, decouple it from Coin Selection by furszy)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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.

DrahtBot cross-referenced this on Oct 4, 2022 from issue wallet: #25768 follow ups by stickies-v

DrahtBot cross-referenced this on Oct 4, 2022 from issue [WIP] wallet: standardize change output detection process by furszy

fanquake force-pushed on Oct 4, 2022

aureleoules commented at 9:17 AM on October 4, 2022: member

Should these comments be enforced with CommentBoolLiterals, CommentIntegerLiterals, CommentFloatLiterals, etc.? https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html

in src/qt/addresstablemodel.cpp:360 in 1b85567c73 outdated

 356 | @@ -357,7 +357,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
 357 |          // Check for duplicate addresses
 358 |          {
 359 |              if (walletModel->wallet().getAddress(
 360 | -                    DecodeDestination(strAddress), /* name= */ nullptr, /* is_mine= */ nullptr, /* purpose= */ nullptr))
 361 | +                    DecodeDestination(strAddress), /*name=*/nullptr, /*is_mine=*/nullptr, /*purpose=*/nullptr))

maflcko commented at 2:50 PM on October 4, 2022:

there is nothing wrong with this

maflcko commented at 2:51 PM on October 4, 2022: member

looks like the only purpose of a bunch of this is to cause needless merge conflicts and backport issues

DrahtBot cross-referenced this on Oct 4, 2022 from issue wallet, rpc: add `label` to `listsinceblock` by brunoerg

DrahtBot cross-referenced this on Oct 4, 2022 from issue psbt: Only include PSBT_OUT_TAP_TREE when the output has a script path by achow101

DrahtBot cross-referenced this on Oct 5, 2022 from issue rpc/wallet: Add details and duplicate section for simulaterawtransaction by anibilthare

DrahtBot cross-referenced this on Oct 5, 2022 from issue wallet: Introduce AddressBookManager by furszy

DrahtBot cross-referenced this on Oct 5, 2022 from issue New `outputs` argument for `bumpfee`/`psbtbumpfee` by rodentrabies

DrahtBot cross-referenced this on Oct 5, 2022 from issue wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101

DrahtBot cross-referenced this on Oct 5, 2022 from issue Rework logging timer by maflcko

DrahtBot cross-referenced this on Oct 5, 2022 from issue [Draft / POC] Silent Payments by w0xlt

DrahtBot cross-referenced this on Oct 6, 2022 from issue rpc: add require_checksum flag to deriveaddresses by kallewoof

DrahtBot cross-referenced this on Oct 6, 2022 from issue Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference by mjdietzx

DrahtBot cross-referenced this on Oct 6, 2022 from issue descriptors: Be able to specify change and receiving in a single descriptor string by achow101

DrahtBot cross-referenced this on Oct 6, 2022 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky

DrahtBot cross-referenced this on Oct 6, 2022 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky

DrahtBot cross-referenced this on Oct 6, 2022 from issue util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky

DrahtBot cross-referenced this on Oct 6, 2022 from issue refactor: Remove settings merge reverse precedence code by ryanofsky

DrahtBot cross-referenced this on Oct 6, 2022 from issue refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky

fanquake force-pushed on Oct 11, 2022

fanquake commented at 1:45 PM on October 11, 2022: member

looks like the only purpose of a bunch of this is to cause needless merge conflicts and backport issues

The easiest way to know the number of conflicts was to just open a PR, and doing that, might as well "fix" all issues, and see how bad it is. In any case, the change here is now much reduced.

DrahtBot added the label Needs rebase on Oct 13, 2022

fanquake force-pushed on Oct 14, 2022

DrahtBot removed the label Needs rebase on Oct 14, 2022

DrahtBot cross-referenced this on Oct 17, 2022 from issue refactor: Use util::Result class for wallet loading by ryanofsky

DrahtBot cross-referenced this on Oct 18, 2022 from issue Multiprocess bitcoin by ryanofsky

DrahtBot cross-referenced this on Oct 21, 2022 from issue Add Import to Wallet GUI by KolbyML

DrahtBot cross-referenced this on Oct 28, 2022 from issue wallet: group outputs only once, decouple it from Coin Selection by furszy

fanquake closed this on Dec 5, 2022

in src/wallet/wallet.cpp:2466 in 14f9dc4bb9 outdated

2380 | @@ -2381,7 +2381,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
2381 |  util::Result<CTxDestination> CWallet::GetNewDestination(const OutputType type, const std::string label)
2382 |  {
2383 |      LOCK(cs_wallet);
2384 | -    auto spk_man = GetScriptPubKeyMan(type, false /* internal */);
2385 | +    auto spk_man = GetScriptPubKeyMan(type, /*internal=*/false);

maflcko commented at 3:46 PM on December 5, 2022:

No objection to the changes of this kind, btw

fanquake commented at 3:57 PM on December 5, 2022:

Ok. They are all changes of this kind now.

fanquake reopened this on Dec 5, 2022

Fixup clang-tidy named argument comments

Fix comments so they are checked/consistent.
Fix incorrect arguments.

203886c443

fanquake force-pushed on Dec 5, 2022

maflcko approved

maflcko commented at 3:57 PM on December 5, 2022: member

ACK. This allows clang-tidy to check that the arg name is correct

Unrelated whitespace changes have been dropped

hebasto commented at 4:48 PM on December 5, 2022: member

This allows clang-tidy to check that the arg name is correct

Is it enforced somehow?

fanquake commented at 4:51 PM on December 5, 2022: member

Is it enforced somehow?

clang-tidy & bugprone-argument-comment

hebasto commented at 4:59 PM on December 5, 2022: member

Is it enforced somehow?

clang-tidy & bugprone-argument-comment

Why does it not fail currently on the master branch, considering https://github.com/bitcoin/bitcoin/blob/38cbf43dee9203364d429dc2179772eca80d8965/src/.clang-tidy#L14-L15 ?

fanquake commented at 5:00 PM on December 5, 2022: member

There are lots of linting related things that are not hard failures on the master branch.

maflcko commented at 8:34 AM on December 6, 2022: member

Why does it not fail currently on the master branch

clang-tidy does not understand the syntax. Fixing the syntax is the goal of this pull

hebasto commented at 9:58 AM on December 6, 2022: member

Why does it not fail currently on the master branch

clang-tidy does not understand the syntax. Fixing the syntax is the goal of this pull

Ah, right. I tested it a bit, and the clang-tidy is able to catch errors like that:

/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/coincontroldialog.cpp:480:102: error: argument name 'noreason' in comment does not match parameter name 'reason' [bugprone-argument-comment,-warnings-as-errors]
        nPayFee = model->wallet().getMinimumFee(nBytes, m_coin_control, /*returned_target=*/nullptr, /*noreason=*/nullptr);
                                                                                                     ^~~~~~~~~~~~~
                                                                                                     /*reason=*/
./interfaces/wallet.h:248:20: note: 'reason' declared here
        FeeReason* reason) = 0;
--

and

/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/rpc/backup.cpp:187:97: error: argument name 'timestampmp' in comment does not match parameter name 'timestamp' [bugprone-argument-comment,-warnings-as-errors]
                pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))}, /*timestampmp=*/0);
                                                                                                ^~~~~~~~~~~~~~~~
                                                                                                /*timestamp=*/
./wallet/wallet.h:602:65: note: 'timestamp' declared here
    bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
--

Concept ACK.

hebasto approved

hebasto commented at 10:23 AM on December 6, 2022: member

ACK 203886c443c4ad76f8a1dba740a286e383e55206, I have reviewed the code and it looks OK, I agree it can be merged.

maflcko merged this on Dec 6, 2022

maflcko closed this on Dec 6, 2022

fanquake deleted the branch on Dec 6, 2022

sidhujag referenced this in commit dcaf34d20a on Dec 6, 2022

bitcoin locked this on Dec 6, 2023