Skip to content

Conversation

@practicalswift
Copy link
Contributor

Don't use zero as null pointer constant.

From the developer notes:

nullptr is preferred over NULL or (void*)0.

From the C++ Core Guidelines:

ES.47: Use nullptr rather than 0 or NULL
Reason Readability. Minimize surprises: nullptr cannot be confused with an int. nullptr also has a well-specified (very restrictive) type, and thus works in more scenarios where type deduction might do the wrong thing on NULL or 0.

Found by compiling the project with -Wzero-as-null-pointer-constant and fixing where appropriate (skipping false positives).

@practicalswift practicalswift force-pushed the zero-as-null-pointer-constant branch from fc6c44b to f0ea2eb Compare July 30, 2018 10:50
@promag
Copy link
Contributor

promag commented Jul 30, 2018

Concept ACK.

@DrahtBot
Copy link
Contributor

Note to reviewers: This pull request conflicts with the following ones:

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
Copy link
Member

If this is going to be done, shouldn't -Wzero-as-null-pointer-constant or a similar sort of lint script be added to the build system to try and stop it's usage re-occurring? Otherwise this seems like it could end up similar to -Wshadow.

@practicalswift
Copy link
Contributor Author

@fanquake Yes that would be the best way to make sure we don't regress! Unfortunately -Wzero-as-null-pointer-constant generates some false positives in our code and some true positives in our dependencies.

@maflcko
Copy link
Member

maflcko commented Jul 30, 2018

Unfortunately -Wzero-as-null-pointer-constant generates some false positives in our code and some true positives in our dependencies.

In which case I suggest to do the same as with -Wshadow: Don't enforce it and don't change existing code. So I tend to Concept NACK here.

@practicalswift
Copy link
Contributor Author

practicalswift commented Jul 30, 2018

@MarcoFalke Oh, I'd love enabling that warning but in contrast to shadowing I think the probably of regressions is much lower – hopefully making this a one time "switch to C++11" change. New code is unlikely to use 0 instead of the proper pointer literal nullptr, no?

I'd assume the instances changed here are from the pre-C++11 era.

@practicalswift practicalswift changed the title Don't use zero as null pointer constant Don't use zero as null pointer constant. Use nullptr (C++11). Jul 30, 2018
@maflcko
Copy link
Member

maflcko commented Jul 30, 2018

Fine, nullptr is type safe, so at least it is an improvement and also doesn't hurt. There shouldn't be any issues given that this compiles, right?

@practicalswift
Copy link
Contributor Author

@MarcoFalke Correct! Type safety is our friend.

This change is safe:

  • Changing from 0 (or NULL) to nullptr where the zero is used as a null pointer constant is always safe.
  • Changing from 0 (or NULL) to nullptr where the zero is NOT used as a null pointer constant will result in a compiler error.

So yes, if it compiles it is safe :-)

@practicalswift practicalswift force-pushed the zero-as-null-pointer-constant branch from f0ea2eb to 08c2760 Compare July 30, 2018 16:34
@kallewoof
Copy link
Contributor

Weak concept NAK, but I'm okay with it if others are (I understand it's safe if it compiles).

Too many files changed (100 lines over 58 files) for a change that isn't really a fix (the reason stated in C++ guidelines is 'readability', which is rather weak for a 58 file change IMO). I feel this is one of those things that can be fixed gradually over time as people update the transgressing code.

@practicalswift
Copy link
Contributor Author

Closing this PR due to lack of consensus ACK :-)

@practicalswift practicalswift deleted the zero-as-null-pointer-constant branch April 10, 2021 19:35
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants