Enable clang-tidy bugprone-argument-comment and fix violations #22903

1. 
   MarcoFalke commented at 3:35 pm on September 6, 2021: member

   Named arguments can be dangerous when they are wrong, because they are not enforced by the compiler. Currently there are only minor typos, no actual bugs.

   Fix the typos and add the .clang-tidy file to make it easier to find them in the future.

2. MarcoFalke added the label Refactoring on Sep 6, 2021
3. 
   MarcoFalke commented at 3:45 pm on September 6, 2021: member

   To test on Ubuntu:

   0apt install clang-tidy bear clang
   1./autogen.sh && ./configure --with-incompatible-bdb CC=clang CXX=clang++
   2make clean && bear make -j $(nproc)     # For bear 2.x
   3make clean && bear -- make -j $(nproc)  # For bear 3.x
   4( cd ./src/ && run-clang-tidy  -j $(nproc) )
   

4. 
   kiminuo commented at 8:28 pm on September 6, 2021: contributor

   Is it possibly instead of:

   make clean && bear make -j $(nproc) -> make clean && bear -- make -j $(nproc)

   ?

   (https://github.com/rizsotto/Bear#how-to-use)

   edit: I think there are two more instances where the parameter names are not entirely correct:

   • https://github.com/MarcoFalke/bitcoin-core/blob/fa91f31752e9387548fa6b932162a12d22c8e211/src/qt/test/addressbooktests.cpp#L115
   • https://github.com/MarcoFalke/bitcoin-core/blob/fa91f31752e9387548fa6b932162a12d22c8e211/src/qt/test/wallettests.cpp#L170
5. 
   DrahtBot commented at 0:26 am on September 7, 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:

   • #22019 (wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101)

   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.

6. 
   MarcoFalke commented at 6:58 am on September 7, 2021: member

   @kiminuo

   No, that doesn’t work for me:

    0bear: Internal error.
    1Traceback (most recent call last):
    2  File "/usr/local/bin/bear", line 265, in wrapper
    3    return function(*args, **kwargs)
    4  File "/usr/local/bin/bear", line 291, in intercept_build
    5    exit_code, current = capture(args, category)
    6  File "/usr/local/bin/bear", line 315, in capture
    7    exit_code = run_build(args.build, env=environment)
    8  File "/usr/local/bin/bear", line 189, in run_build
    9    exit_code = subprocess.call(command, *args, **kwargs)
   10  File "/usr/lib64/python3.9/subprocess.py", line 349, in call
   11    with Popen(*popenargs, **kwargs) as p:
   12  File "/usr/lib64/python3.9/subprocess.py", line 951, in __init__
   13    self._execute_child(args, executable, preexec_fn, close_fds,
   14  File "/usr/lib64/python3.9/subprocess.py", line 1821, in _execute_child
   15    raise child_exception_type(errno_num, err_msg, err_filename)
   16FileNotFoundError: [Errno 2] No such file or directory: '--'
   17bear: Please run this command again and turn on verbose mode (add '-vvvv' as argument).
   

7. 
   kiminuo commented at 7:02 am on September 7, 2021: contributor

   My bear version is bear 3.0.8 and I have Ubuntu 21.04. Anyway, bear make -j $(nproc) does not work for me, only bear -- make -j $(nproc). Maybe there was a breaking change in bear command. I don’t know.

   edit: Yes, https://github.com/rizsotto/Bear/pull/388/files

8. 
   MarcoFalke commented at 7:10 am on September 7, 2021: member

   @kiminuo Thanks, updated comment
9. Enable clang-tidy bugprone-argument-comment and fix violations fa57fa1a2e
10. MarcoFalke force-pushed on Sep 7, 2021
11. 
    MarcoFalke commented at 8:47 am on September 7, 2021: member

    edit: I think there are two more instances where the parameter names are not entirely correct:

    Thanks, fixed

12. 
    practicalswift commented at 7:57 am on September 8, 2021: contributor

    Concept ACK

    Could add this specific clang-tidy check as part of the CI lint pass to reduce the risk of introduction of bugprone argument comments going forward?

13. 
    MarcoFalke commented at 8:03 am on September 8, 2021: member

    Could add this specific clang-tidy check as part of the CI lint pass to reduce the risk of introduction of bugprone argument comments going forward?

    Seems like something that should be done in a separate pull request to keep the discussion here focussed on adding the flag and fixing the violations.

14. 
    practicalswift commented at 10:27 am on September 8, 2021: contributor

    cr ACK fa57fa1a2e764792b873998ddf38db2ac061dcb6

    Could add this specific clang-tidy check as part of the CI lint pass to reduce the risk of introduction of bugprone argument comments going forward?

    Seems like something that should be done in a separate pull request to keep the discussion here focussed on adding the flag and fixing the violations.

    Makes sense! I’d gladly review such PR FWIW :)

15. fanquake approved
16. 
    fanquake commented at 3:33 am on September 9, 2021: member

    ACK fa57fa1a2e764792b873998ddf38db2ac061dcb6
17. fanquake merged this on Sep 9, 2021
18. fanquake closed this on Sep 9, 2021

    ---

19. sidhujag referenced this in commit daa3c6d230 on Sep 11, 2021
20. MarcoFalke deleted the branch on Sep 14, 2021
21. DrahtBot locked this on Oct 30, 2022

Contributors
MarcoFalke kiminuo DrahtBot practicalswift fanquake

Labels
Refactoring