Skip to content
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.

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

refactor: NULL => nullptr #2256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

scivision
Copy link
Contributor

@scivision scivision commented Feb 15, 2023

this was done with IDE find and replace NULL => nullptr
To me, there were too many lints that had accumulated to use clang-tidy directly.

if this makes a PR conflict, it may only be on a couple lines, and they could likewise do find/replace.

@scivision scivision force-pushed the nullptr branch 3 times, most recently from f91bfa1 to 7462c58 Compare February 15, 2023 02:36
.clang-tidy Outdated Show resolved Hide resolved
@digit-google
Copy link
Contributor

This is nice! I would suggest two small things though:

  1. Provide clear instructions on how to reproduce this locally, so that developers can easily update their own patches on top of this change once it is submitted.

  2. Apply git clang-format just after the clang-tidy step, and add that in the commit message as well.

For example, on my Debian/testing machine, I could do the following:

clang-tidy-14 --checks=-*,modernize-use-nullptr --fix-errors src/*.cc src/*.h -- std=c++11
git clang-format

Using "-*" at the start of --checks is necessary to disable all the checks in the .clang-tidy file, since there are tons of unrelated errors trigerred by the source code if I do not use this.

@scivision scivision force-pushed the nullptr branch 4 times, most recently from b056664 to b6a2d92 Compare April 16, 2024 03:34
now that C++11 is used in Ninja, use nullptr
@jhasse
Copy link
Collaborator

jhasse commented Apr 27, 2024

The change to .clang-tidy seems to be gone?

Also can you edit lexer.cc and disk_interface.cc without the whitespace changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants