Skip to content

[ML] Use nullptr instead of 0 in core lib #35

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

Merged
merged 3 commits into from
Apr 9, 2018
Merged

[ML] Use nullptr instead of 0 in core lib #35

merged 3 commits into from
Apr 9, 2018

Conversation

davidkyle
Copy link
Member

The latest version of llvm on macos will warn if 0 is used instead of nullptr (-Wzero-as-null-pointer-constant). GCC already has a similar warning.

This PR replaces 0 with nullptr I intend to do the entire code base but this change is already large so I will break it up by the libraries.

Backport pending

@@ -61,25 +61,30 @@ namespace ml
namespace core
{

#ifdef MacOSX
CThread::TThreadId CThread::UNALLOCATED_THREAD_ID = nullptr;
Copy link
Member Author

Choose a reason for hiding this comment

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

On macos _darwin_pthread_t is defined to be a pointer see /usr/include/sys/_pthread/_pthread_types.h on Linux pthread_t is an arithmetic type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could just write this:

CThread::TThreadId CThread::UNALLOCATED_THREAD_ID{};

and avoid the #ifdef.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@davidkyle
Copy link
Member Author

@edsavage could you review this please. Rebasing on the code style commit introduced a lot of chanages

@droberts195
Copy link
Contributor

I think it's worth backporting this to 6.3, otherwise it will cause clashes in other backports.

@davidkyle davidkyle merged commit 6b64c63 into elastic:master Apr 9, 2018
@davidkyle davidkyle deleted the zero-to-nullptr branch April 9, 2018 08:56
@sophiec20 sophiec20 added the :ml label Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants