clang-tidy: fixup named argument comments

[fanquake]

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

[DrahtBot]

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.

[aureleoules]

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

[maflcko]

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

[fanquake]

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.

[maflcko]

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

Unrelated whitespace changes have been dropped

[hebasto]

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

Is it enforced somehow?

[fanquake]

Is it enforced somehow?

clang-tidy & bugprone-argument-comment

[hebasto]

Is it enforced somehow?

clang-tidy & bugprone-argument-comment

Why does it not fail currently on the master branch, considering

bitcoin/src/.clang-tidy

Lines 14 to 15 in 38cbf43

	WarningsAsErrors: '
	bugprone-argument-comment,

?

[fanquake]

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

[maflcko]

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]

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]

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