C/C++ Checking for NULL pointer

Question

In a recent code review, a contributor is trying to enforce that all NULL checks on pointers be performed in the following manner:

int * some_ptr;
// ...
if( some_ptr == NULL )
{
  // handle null-pointer error
}
else
{
  // proceed
}

instead of

int * some_ptr;
// ...
if( some_ptr )
{
  // proceed
}
else
{
  //handle null-pointer error
}

I agree that his way is a little more clear in the sense that it's explicitly saying "Make sure this pointer is not NULL" but I would counter that by saying that anyone who's working on this code would understand that using a pointer variable in an if statement is implicitly checking for NULL. Also I feel the second method has a smaller chance of introducing a bug of the ilk:

if( some_ptr = NULL )

which is just an absolute pain to find and debug.

Can anyone weigh in on which way you prefer and why?

Answer 1

if (foo) is clear enough. Use it.

Answer 2

Most compilers I've used will at least warn on the if assignment without further syntax sugar so I don't buy that argument. That said, I've used both prfessionally and have no preference for either. The == NULL is definately clearer though in my opinion.

Answer 3

The following is a common practice, clear, and safe from your 2nd scenario. Don't always assume what someone else will assume when looking at your code.

int * some_ptr; 
// ... 
if( NULL == some_ptr ) 
{ 
  // handle null-pointer error 
} 
else 
{ 
  // proceed 
} 

Answer 4

In my experience, tests of the form if (ptr) or if (!ptr) are preferred. They do not depend on the definition of the symbol NULL. They do not expose the opportunity for the accidental assignment. And they are clear and succinct.

Edit: As SoapBox points out in a comment, they are compatible with C++ classes such as auto_ptr that are objects that act as pointers and which provide a conversion to bool to enable exactly this idiom. For these objects, an explicit comparison to NULL would have to invoke a conversion to pointer which may have other semantic side effects or be more expensive than the simple existence check that the bool conversion implies.

I have a preference for code that says what it means without unneeded text. if (ptr != NULL) has the same meaning as if (ptr) but at the cost of redundant specificity. The next logical thing is to write if ((ptr != NULL) == TRUE) and that way lies madness. The C language is clear that a boolean tested by if, while or the like has a specific meaning of non-zero value is true and zero is false. Redundancy does not make it clearer.

Answer 5

I use if (ptr), but this is completely not worth arguing about.

I like my way because it's concise, though others say == NULL makes it easier to read and more explicit. I see where they're coming from, I just disagree the extra stuff makes it any easier. (I hate the macro, so I'm biased.) Up to you.

I disagree with your argument. If you're not getting warnings for assignments in a conditional, you need to turn your warning levels up. Simple as that. (And for the love of all that is good, don't switch them around.)

Note in C++0x, we can do if (ptr == nullptr), which to me does read nicer. (Again, I hate the macro. But nullptr is nice.) I still do if (ptr), though, just because it's what I'm used to.

Answer 6

Actually, I use both variants.

There are situations, where you first check for the validity of a pointer, and if it is NULL, you just return/exit out of a function. (I know this can lead to the discussion "should a function have only one exit point")

Most of the time, you check the pointer, then do what you want and then resolve the error case. The result can be the ugly x-times indented code with multiple if's.

Answer 7

I do the

if (ptr)
{
// do stuff
}

method as well. I mostly do it out of habit and because I expect the developers I work with to understand the meaning. It also saves me a little typing.

Also, if comparing values, I prefer to put the constant first in the equality check, to prevent situations of assignment rather than equality checking.

if ( 0 == ptr )
{
    // error
}

Answer 8

I think this is quite clear:

if( !some_ptr )
{
  // handle null-pointer error
}
else
{
  // proceed
}

Read it as "If there is no pointer ..."

Additionally it is concise and has no danger of accidental assignments.

Answer 9

I'll start off with this: consistency is king, the decision is less important than the consistency in your code base.

In C++

NULL is defined as 0 or 0L in C++.

If you've read The C++ Programming Language Bjarne Stroustrup suggests using 0 explicitly to avoid the NULL macro when doing assignment, I'm not sure if he did the same with comparisons, it's been a while since I read the book, I think he just did if(some_ptr) without an explicit comparison but I am fuzzy on that.

The reason for this is that the NULL macro is deceptive (as nearly all macros are) it is actually 0 literal, not a unique type as the name suggests it might be. Avoiding macros is one of the general guidelines in C++. On the other hand, 0 looks like an integer and it is not when compared to or assigned to pointers. Personally I could go either way, but typically I skip the explicit comparison (though some people dislike this which is probably why you have a contributor suggesting a change anyway).

Regardless of personal feelings this is largely a choice of least evil as there isn't one right method.

This is clear and a common idiom and I prefer it, there is no chance of accidentally assigning a value during the comparison and it reads clearly:

if(some_ptr){;}

This is clear if you know that some_ptr is a pointer type, but it may also look like an integer comparison:

if(some_ptr != 0){;}

This is clear-ish, in common cases it makes sense... But it's a leaky abstraction, NULL is actually 0 literal and could end up being misused easily:

if(some_ptr != NULL){;}

C++0x has nullptr which is now the preferred method as it is explicit and accurate, just be careful about accidental assignment:

if(some_ptr != nullptr){;}

Until you are able to migrate to C++0x I would argue it's a waste of time worrying about which of these methods you use, they are all insufficient which is why nullptr was invented (along with generic programming issues which came up with perfect forwarding.) The most important thing is to maintain consistency.

In C

C is a different beast.

In C NULL can be defined as 0 or as ((void *)0), C99 allows for implementation defined null pointer constants. So it actually comes down to the implementation's definition of NULL and you will have to inspect it in your standard library.

Macros are very common and in general they are used a lot to make up for deficiencies in generic programming support in the language and other things as well. The language is much simpler and reliance on the pre-processor more common.

From this perspective I'd probably recommend using the NULL macro definition in C.

Answer 10

k&r would have you check for null == ptr to avoid an accidental assignment

Answer 11

This is one of the fundamentals of both languages that pointers evaluate to a type and value that can be used as a control expression, bool in C++ and int in C. Just use it.

Answer 12

Personally I've always used if (ptr == NULL) because it makes my intent explicit, but at this point it's just a habit.

Using = in place of == will be caught by any competent compiler with the correct warning settings.

The important point is to pick a consistent style for your group and stick to it. No matter which way you go, you'll eventually get used to it, and the loss of friction when working in other people's code will be welcome.

Answer 13

Frankly, I don't see why it matters. Either one is quite clear and anyone moderately experienced with C or C++ should understand both. One comment, though:

If you plan to recognize the error and not continue executing the function (i.e., you are going to throw an exception or return an error code immediately), you should make it a guard clause:

int f(void* p)
{
    if (!p) { return -1; }

    // p is not null
    return 0;
}

This way, you avoid "arrow code."

Answer 14

If style and format are going to be part of your reviews, there should be an agreed upon style guide to measure against. If there is one, do what the style guide says. If there's not one, details like this should be left as they are written. It's a waste of time and energy, and distracts from what code reviews really ought to be uncovering. Seriously, without a style guide I would push to NOT change code like this as a matter of principle, even when it doesn't use the convention I prefer.

And not that it matters, but my personal preference is if (ptr). The meaning is more immediately obvious to me than even if (ptr == NULL).

Maybe he's trying to say that it's better to handle error conditions before the happy path? In that case I still don't agree with the reviewer. I don't know that there's an accepted convention for this, but in my opinion the most "normal" condition ought to come first in any if statement. That way I've got less digging to do to figure out what the function is all about and how it works.

The exception to this is if the error causes me to bail from the function, or I can recover from it before moving on. In those cases, I do handle the error first:

if (error_condition)
  bail_or_fix();
  return if not fixed;

// If I'm still here, I'm on the happy path

By dealing with the unusual condition up front, I can take care of it and then forget about it. But if I can't get back on the happy path by handling it up front, then it should be handled after the main case because it makes the code more understandable. In my opinion.

But if it's not in a style guide then it's just my opinion, and your opinion is just as valid. Either standardize or don't. Don't let a reviewer pseudo-standardize just because he's got an opinion.

Answer 15

• Pointers are not booleans
• Modern C/C++ compilers emit a warning when you write if (foo = bar) by accident.

Therefore I prefer

if (foo == NULL)
{
    // null case
}
else
{
    // non null case
}

or

if (foo != NULL)
{
    // non null case
}
else
{
    // null case
}

However, if I were writing a set of style guidelines I would not be putting things like this in it, I would be putting things like:

Make sure you do a null check on the pointer.

Answer 16

I'm a huge fan of the fact that C/C++ doesn't check types in the boolean conditions in if, for and while statements. I always use the following:

if (ptr)

if (!ptr)

even on integers or other type that converts to bool:

while(i--)
{
    // Something to do i times
}

while(cin >> a >> b)
{
    // Do something while you've input
}

Coding in this style is more readable and clearer to me. Just my personal opinion.

Recently, while working on OKI 431 microcontroller, I've noticed that the following:

unsigned char chx;

if (chx) // ...

is more efficient than

if (chx == 1) // ...

because in later case the compiler has to compare the value of chx to 1. Where chx is just a true/false flag.