Skip to content

clang-tidy: Fix modernize-use-nullptr in headers #26708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 17, 2022
Merged

clang-tidy: Fix modernize-use-nullptr in headers #26708

merged 1 commit into from
Dec 17, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Dec 15, 2022

Split from #26705 as was requested in #26705 (comment).

To test this PR, consider applying a diff as follows:

--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -12,17 +12,9 @@ readability-redundant-declaration,
 readability-redundant-string-init,
 '
 WarningsAsErrors: '
-bugprone-argument-comment,
-bugprone-use-after-move,
-misc-unused-using-decls,
-modernize-use-default-member-init,
 modernize-use-nullptr,
-performance-for-range-copy,
-performance-move-const-arg,
-performance-unnecessary-copy-initialization,
-readability-redundant-declaration,
-readability-redundant-string-init,
 '
 CheckOptions:
  - key: performance-move-const-arg.CheckTriviallyCopyableMove
    value: false
+HeaderFilterRegex: '.'

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 15, 2022 •
edited
Loading

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 john-moffett
Concept ACK kevkevinpal

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:

  • #26705 (clang-tidy: Check headers and fixes them by hebasto)

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.

@hebasto hebasto mentioned this pull request Dec 15, 2022
Merged
@kevkevinpal
Copy link
Contributor

kevkevinpal commented Dec 16, 2022 •
edited
Loading

ACK but I'm going to try and run clang-tidy myself, first time doing so it might take me a bit

@kevkevinpal
Copy link
Contributor

was able to run clang-tidy but I only saw a message for this part of the code
FormatListN() : FormatList(nullptr, 0) {}
not sure how to get clang-tidy to complain about the 3 NULL values changed to nullptr

@hebasto
Copy link
Member Author

hebasto commented Dec 16, 2022 •
edited
Loading

@kevkevinpal

not sure how to get clang-tidy to complain about the 3 NULL values changed to nullptr

To observe clang-tidy's errors, one could apply to the master branch a diff as follows:

--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -12,17 +12,9 @@ readability-redundant-declaration,
 readability-redundant-string-init,
 '
 WarningsAsErrors: '
-bugprone-argument-comment,
-bugprone-use-after-move,
-misc-unused-using-decls,
-modernize-use-default-member-init,
 modernize-use-nullptr,
-performance-for-range-copy,
-performance-move-const-arg,
-performance-unnecessary-copy-initialization,
-readability-redundant-declaration,
-readability-redundant-string-init,
 '
 CheckOptions:
  - key: performance-move-const-arg.CheckTriviallyCopyableMove
    value: false
+HeaderFilterRegex: '.'

and, for example, run a CI task locally:

$ MAKEJOBS="-j16" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh

or in a personal Cirrus account: https://cirrus-ci.com/task/4659814702252032.

My local run has in output:

/home/hebasto/git/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./tinyformat.h:511:23: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
            : m_value(NULL),
                      ^~~~
                      nullptr
/home/hebasto/git/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./tinyformat.h:512:26: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
            m_formatImpl(NULL),
                         ^~~~
                         nullptr
/home/hebasto/git/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./tinyformat.h:513:25: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
            m_toIntImpl(NULL)
                        ^~~~
                        nullptr
/home/hebasto/git/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./tinyformat.h:1008:40: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
    public: FormatListN() : FormatList(0, 0) {}
                                       ^
                                       nullptr

john-moffett
john-moffett approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@john-moffett john-moffett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK adb7dba

@maflcko maflcko merged commit 6c01323 into bitcoin:master Dec 17, 2022
@hebasto hebasto deleted the 221215-tidy-null branch December 17, 2022 11:34
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 17, 2022
Merge bitcoin#26708: clang-tidy: Fix modernize-use-nullptr in headers
fd665e9 
adb7dba clang-tidy: Fix `modernize-use-nullptr` in headers (Hennadii Stepanov)

Pull request description:

  Split from bitcoin#26705 as was requested in bitcoin#26705 (comment).

  To test this PR, consider applying a diff as follows:
  ```diff
  --- a/src/.clang-tidy
  +++ b/src/.clang-tidy
  @@ -12,17 +12,9 @@ readability-redundant-declaration,
   readability-redundant-string-init,
   '
   WarningsAsErrors: '
  -bugprone-argument-comment,
  -bugprone-use-after-move,
  -misc-unused-using-decls,
  -modernize-use-default-member-init,
   modernize-use-nullptr,
  -performance-for-range-copy,
  -performance-move-const-arg,
  -performance-unnecessary-copy-initialization,
  -readability-redundant-declaration,
  -readability-redundant-string-init,
   '
   CheckOptions:
    - key: performance-move-const-arg.CheckTriviallyCopyableMove
      value: false
  +HeaderFilterRegex: '.'
  ```

ACKs for top commit:
  john-moffett:
    ACK adb7dba

Tree-SHA512: 67241fb212d837157a0a26f0d59e7f30a9d270d5b0ebfeb6ad9631e460fc7fba8c9a9dcd4c0520789353f68025a9f090f40f17176472a93cce1411e6d56f930b
@kevkevinpal
Copy link
Contributor

@hebasto thanks I think I was able to figure it out, I ran the CI task locally and then did docker exec -it <containerid> /bin/bash then ran clang-tidy ./src/tinyformat.h and was able to get the correct warning message

@bitcoin bitcoin locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@kevkevinpal kevkevinpal kevkevinpal approved these changes

@john-moffett john-moffett john-moffett approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @DrahtBot @kevkevinpal @john-moffett @maflcko