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

Remove unnecessary null pointer checks #7432

Closed
elfring opened this issue Jan 28, 2016 · 10 comments
Closed

Remove unnecessary null pointer checks #7432

elfring opened this issue Jan 28, 2016 · 10 comments

Comments

@elfring
Copy link

elfring commented Jan 28, 2016

An extra null pointer check is not needed in functions like the following.

I reported this issue for another evolving software before.

@jonasschnelli
Copy link
Contributor

At least a if NULL check does not hurt.
But I agree. Feel free do open a PR.

@elfring
Copy link
Author

elfring commented Jan 28, 2016

Would you also like to update another software library?

@laanwj
Copy link
Member

laanwj commented Jan 28, 2016

In Bitcoin Core we believe in belt-and-suspenders checks. A small change to the code can turn a 'unnecessary' NULL pointer check into the only thing that prevents a horrible security issue. Checking, even though not strictly necessary, may add some robustness to the API.

Also it may be hard to prove that a check is unnecessary, evaluating the code paths is a lot of review work while there are tons of more serious issues.

So let's just leave this as it is. If you can quantify that it saves time in a critical path (say an inner loop that is executed millions of times), feel free to remove an unnecessary check, but don't do so indiscriminately.

@elfring
Copy link
Author

elfring commented Jan 28, 2016

Checking, even though not strictly necessary, may add some robustness to the API.

Do you imagine any special case for the eventual handling of C++ compilers with a standard-incompliant implementation of the delete operator?

@laanwj
Copy link
Member

laanwj commented Jan 28, 2016

Sure, if you encounter the code

if (x)
    delete x;

Then, according to the C++ standard, you can replace it with

delete x;

But don't do so if there is more code in the if ().

@laanwj laanwj closed this as completed Jan 28, 2016
@elfring
Copy link
Author

elfring commented Jan 28, 2016

Why did you close this issue already?

@laanwj
Copy link
Member

laanwj commented Jan 28, 2016

I don't see this as a serious issue.
A few extra checks won't cause bugs, nor (likely) affect performance.

@elfring
Copy link
Author

elfring commented Jan 28, 2016

I suggested just another small software optimisation here.

@jonasschnelli
Copy link
Contributor

Please no bikeshedding on avoidable pointer checks. No need to create an issue for that (create a PR instead if you want to improve things).

@maflcko
Copy link
Member

maflcko commented Jan 28, 2016

Isn't the compiler taking care of such?

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants