clang-tidy: fixup named argument comments

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.

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

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

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:

0/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]
1        nPayFee = model->wallet().getMinimumFee(nBytes, m_coin_control, /*returned_target=*/nullptr, /*noreason=*/nullptr);
2                                                                                                     ^~~~~~~~~~~~~
3                                                                                                     /*reason=*/
4./interfaces/wallet.h:248:20: note: 'reason' declared here
5        FeeReason* reason) = 0;
6--

and

0/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]
1                pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))}, /*timestampmp=*/0);
2                                                                                                ^~~~~~~~~~~~~~~~
3                                                                                                /*timestamp=*/
4./wallet/wallet.h:602:65: note: 'timestamp' declared here
5    bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
6--

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

clang-tidy: fixup named argument comments #26238

Reviews

Conflicts