Code Style: pointer comparisons

Technical Feedback
11

Of course I know it is ridiculous. I am saying that your reaction of “ridiculous” to the difference between if(pointer != NULL) and if((pointer == NULL) == false) is exactly the same as my reaction to the difference between if(pointer) and if(pointer != NULL). LOL

1 Like
12

if(pointer) looks shorter in functional way, like in LISP, but if (pointer == nullptr) string makes readability of pointers in massive codeblocks better.
I would prefer the first if I write code, and the second if I read a lot of code.
So, for large-scale projects, the absence of such syntactic sugar makes sense.

13

That’s my point exactly. Code is read so much more than it’s written, readability is paramount.

1 Like
14

The if(pointer) construct is both standard C and a very common C programming idiom.
I am a BIG fan of readability (as noted, code is read much more often than it is written), but I am unconvinced that being extra verbose with if(pointer != NULL) and such is any improvement.

Also, as mentioned, doing the explicit comparison opens up the very common = vs == error. Yes, a friendly compiler will flag the warning, but now we are assuming the programmer is diligent about checking the compiler output for warnings.

1 Like
15

If clarity is what you are after, give the gsl’s not_null a look, it not only makes it clear what cannot be null, it’ll even fail to compile if you obviously violate the rule,. sadly C++ only, and there is nothing for C code in that department.

16

Personally I’m in favor of using implicit null-checks by default, as I’ll explain in the following. However I do sometimes use explicit checks, where the check is an important part of the algorithm. In other words, if the null-check is more than a regular sanity or validation check, I emphasize it with an explicit check. The exception is if I think that makes code less readable.

Reasons against explicit checks

Possible bugs

The issue with accidental assignment when comparison was the intend is only partially solved with the compiler warnings. Cases like this can still happen:

if ((possibly_null = nullptr) && (...)) {
  ...
}

This is a bug not caught by compilers. No warning in GCC for me.
It may happen in other common cases too (!).

Readability

Explicit null-checks introduce a negation, a level of indirection and with that a possible source of confusion. A regex search shows that it’s relatively common to do something like this:

const bool recalc_object = (ob->id.recalc & ID_RECALC_ALL) != 0;
const bool recalc_data = (object_data != NULL) ? ((object_data->recalc & ID_RECALC_ALL) != 0) :                                                 0;

if (!recalc_object && !recalc_data) {
...
}

const bool is_multiview = (camera != NULL) && (scene->r.scemode & R_MULTIVIEW) != 0;

if (!is_multiview) {
  return camera;
}

The explicit comparisons and double negations make this code rather confusing and a source of possible errors. Maybe some people can deal with this better than others.
Now, it’s fair to argue that there are multiple ways to make cases like this more readable. But my point is, it’s a common enough pattern to be a problem.

Alternative solutions

As Sybren noted, for him the purpose of explicit checks is the following question:

Can this pointer be NULL at this point in the code?

A fair and important question.

In C++ there are other ways to answer it. The following is commonly recommended, the C++ Core Guidelines cover it too - roughly:

  • “By default” you should pass objects by (const) & reference (if copies are not a reasonable option).
  • When you have to rebind a reference, use a raw pointer but wrap it into a non_null template if applicable.
  • Optional objects, like return values for lookups should use std::optional<>. If you need an optional reference, use a pointer - that’s what they are for. (std::optional<> for reference types is not covered by the C++ standards and not supported by most compilers)

If applied consistently, most raw-pointer usages should vanish. A raw pointer not marked as non_null should be null-checked before dereferencing.

Unfortunately C gives less options here. I think we should make good use of BLI_assert(). Another thing we can use is ATTR_NONNULL(), which helps a bit but has issues too (e.g. GCC only I think). Documentation can also help a lot.

Again, I’m not entirely against the explicit checks, I use them too sometimes. But I think these points should be considered.

2 Likes
17

This is just a weakness of C/C++, and is one of the reasons some languages use := for assignment instead of =. It’s true that removing the comparison removes the issue that comparisons are notoriously easy to confuse with assignments.

That depends on the complexity of the surrounding code. In many cases explicit comparison would transform this:

if (!camera) {
  return NULL;
}

into this:

if (camera == NULL) {
  return NULL;
}

I don’t see a huge readability difference between the two. The latter does convey more information about what camera actually is.

The is_multiview example can be changed into:

if (camera == NULL) {
  return NULL;
}
if ((scene->r.scemode & R_MULTIVIEW) == 0) {
  return camera;
}

I find that much easier to read.

Just because that the feature is called “multiview” doesn’t mean that the variable should be named as such. You can also do is_singleview = (camera == NULL) || (scene->r.scemode & R_MULTIVIEW) == 0 if you want. Apply De Morgan’s Laws if it makes things simpler.

18

If I’m using nullptr as an indicator of a particular, known state, not just a possible state based on which I do an action like in wavefront_obj_exporter_mesh.cc$318-322, it is more clear if I use == nullptr later on. This conveys that the particular state has been reached. It is no longer a sanity check.

Also, adding to the following,

if I write if(var), am I checking if var is a setting that is set to false ? if var is a character that could be '\0' ? var is an int that changes and may reach 0 ? or a pointer ?
This could be explicitly written as
if (var == false) : already in style guide, not to use 0, 1 for booleans.
if (var == ‘\0’ ), if (var == 0) and if (var == nullptr).

19

Has there been any decision in the meantime or should we just go with the style used in the existing code when writing patches?

20

Go with the surrounding code style for now. I think there are good arguments for either approach. To recap:

  • Some not unimportant people, namely Ton and Sergey, prefer explicit checks. Personally I agree with them. Even in C++ I feel there is a semantic difference between if(!thing) or if (thing == nullptr); to me the former means “thing is not there or empty or some other falsey value” and the other means “thing is a null pointer”. In some cases these are equivalent, in other cases they are not.
  • The C++ Core Guidelines suggest using implicit notation, and others have pointed at other advantages (shorter code, avoiding the assignment-instead-of-comparison bug). These are also important factors.
2 Likes
21

I don’t find “appeal to Authority” a very convincing argument, there’s no point to debate the merits, if ya’ll go “The core devs like this style better and this is what we’re gonna do” fair enough for me, that’s your prerogative, I won’t stand in the way.

However if we do debate merits, the only additional context I see if (thing == nullptr) providing over if(!thing) is that thing can definitely be compared to a pointer type in the if (thing == nullptr) case, however nothing can be said about the type of thing itself.

While it hard to deny it adds information, are if statements really the correct place to convey this additional information? also how important is having this information at the if call site?

I don’t feel it’s very important, but I’m open hear different opinions on this

2 Likes
22

I’m with @julianeisel on this one. I may use explicit != nullptr on occasion, but in general, I try to avoid it. The danger of accidental assignment instead of comparison is real (as evidenced by the commit Julian linked to) and may lead to hard to trace bugs.

23

I find it an important one, but indeed not something that’s enough to sway the discussion one way or the other. If there was no point in discussing the merits, I wouldn’t have started this discussion in the first place.

About if (thing == nullptr) being bug-prone: With the danger of sounding old, I’ve been programming C++ for approx 25 years now, and I think I can count how many times I made that particular mistake on one hand. Yes, the mistake happens, but so do countless others. When it comes to avoiding bugs, I think clarity of code by reduction of complexity and by having a good architectural design is far more important than this entire discussion.

Good question, made me think. Now that I have thought about it, I think it is indeed a nice place to have such information conveyed. After all, it’s the branches in the code that add the complexity. All the nested ifs and fors and whiles add up. It’s exactly at that spot of increased complexity where you want to have as much clarity, and thus as much local understandability. Of course this is again subjective, as some people will say if (thing) is more clear than if (thing != nullptr).

The more I look at the issue, the more I feel unconvinced by either “always implicit” or “always explicit” arguments.

  • if (thing): compact, and positive. This matches what most people think, “if the thing is there”. Could mean multiple things, though; the interpretation depends on the definition of thing. Does it have a convert-to-bool function? If it’s there, is that function NULL-safe? If not, this expression could cause nasty issues. Makes no distinction between “is there but false-like” or “is not there”.
  • if (!thing): compact, but with same issues as above.
  • if (thing != nullptr): explicit, but has a hidden double negation. To my mind this reads as “if the thing is not non-existant”, and I don’t quite like such double negations.
  • if (thing == nullptr): explicit, and without double negation. No questions about what this means.

Personally I would turn the above into a set of heuristics like this:

  • If you want to check for NULL-ness, for example to return a default value when a parameter wasn’t passed, or to perform an allocation, use if (thing == nullptr). Using if (!thing) would cause double allocation if thing was already allocated but empty.
  • If you want to check for “has usable content” (i.e. non-empty string, non-empty list, allocated object, etc.), use if (thing). Using the explicit approach would require two checks here, f.e. if (thing != nullptr && !thing.empty()).
24

“It’s important but not enough to sway anything”, i… what… no… how…nah… i’m out…

I made my argument as best as I can, I disagree with this proposal but do what you must.

25

I’ve been programming for 40 years and don’t think I have ever found if(thing) to be unclear.

Does anyone really look at if(dis && dis.dat && dis.dat.splah) and think it would more clear as if((dis != nullptr) && (dis.dat != nullptr) && (dis.dat.splah != nullptr))

If we really have a preference for the IF statement to give hints about the enclosed variable types, then we should also prefer if(boolthing == true) and if(chararray[0] == \0) or perhaps ensure that all variable names contain their type like if(thing_of_type_int == 1)

26

I don’t must anything. I just want to get noses pointing into the same direction.

@Harleya You’re in favour of implicit checks, that’s fine, you made that point before already. Coming up with ridiculous extrapolations doesn’t help in actually discussing the topic at hand.

It’s clear that most people in this discussion prefer the implict checks; some like them always and some like them in certain cases. If we can’t decide, shall we just stick to the C++ Core Guidelines as @julianeisel described in Code Style: pointer comparisons ?

27

Ah. I did not realize that only people in favor of explicit checks are allowed to comment more than once.

Nor does such rude and defensive dismissal of others’ arguments by calling them “ridiculous” so I will leave you to it.

1 Like
28

Jeesh man. I’ve been trying to summarize, to present a balanced view of pros and cons of either approach, and trying to find some consensus. What I meant to say is that repeating the same thing that was said months ago doesn’t really add anything new.

29

I will always vote for implicit checks just like for booleans, though I already see so much explicit checks in the Blender source.

I remember in the Uni - first year - if you coded in C some condition like…
if (boolean == True)
it was bad, directly, they were really serious, even though it does the same as
if (boolean)
because they wanted you to avoid redundant code,
and the truth is you read both the same but one is more direct and clean, specially for multiple checks…

I could undertand that you want to do explicit checks for pointers if you were using implicit checks for every boolean, just to not mix both and cause confusion, but I see so much redundant explicit checks of non-pointers so I guess that’s not the point? (correct me if I’m wrong).

30

This is exactly why I wanted the discussion: to maybe choose between two approaches that are both currently used in Blender. In light of the discussions so far, it seems like a hard choice between always one or always the other isn’t desirable.

I tried to summarize earlier arguments, and to make some new proposals based on the insight people offered. Nobody is responding to those, though, and we keep going in circles. Because of that, I’ll close this thread. If you want to talk about the subject, DM me here or on Blender Chat.

TL;DR:

If you’re here to just read the final result of this discussion and want to know what’s the accepted thing to do: just follow the C++ Core Guidelines as Julian describes in this comment and you’ll probably be fine. It’s more important that code is clear & understandable than to follow a specific strict guideline.

2 Likes