From what I’ve always understood from @Ton and @sergey, it is preferred to have explicit pointer comparisons. In other words:
void *pointer;
/* Don't: */
if (pointer) …
if (!pointer) …
/* Do in C: */
if (pointer != NULL) …
if (pointer == NULL) …
/* Do in C++: */
if (pointer != nullptr) …
if (pointer == nullptr) …
Shall we make this explicit in the Code Style Guide?
I think it definitely should be explicit if it is expected.
BTW I didn’t want you to ask for approval of this guideline, just announce it in public, so as much devs as possibe are aware of this change.
I know, but it makes the announcement stronger if there is a bit of a public discussion and everybody actually agrees 
Personally not a fan of writing superfluous code, but not my project, so not my code style, so yeah go for it if that’s what you want.
That being said from random sampling our codebase, it feels like if(x) is used more often than if(x == NULL/nullptr) even in the MEM_SAFE_FREE it isn’t used.
Just enforcing this in the codebase this will keep us busy for the next couple of code quality Fridays with absolutely mindbogglingly dumb work that seemingly will benefit no-one…
I disagree with “[this explicitness] will benefit no-one”. To me it pretty much depends on the question that you’re trying to get answered.
The question
What does this line of code do?
is easily answered in both the implicit and explicit style. However, especially when code gets a bit more complex, answering the question
Can this pointer be
NULLat this point in the code?
is easier when you can scan for if … NULL.
Also I’m not saying “this should change in all the code NOW”. If this is what you want to spend your Code Quality Day on, fine by me, but I think there’s sweeter low-hanging fruit to eat first.
And at the same time you introduce a new vector for bugs, it’s real easy to miss an = and end up with
if(x = NULL)
which naturally can be sidestepped by putting the NULL first
if(NULL = x) will give a nice build error, but it it’ll hurt your brain every-time you write it and every time after that when you read it 
but this thread is not about should we do it, this has already been decided apparently so no need to haggle about that.
If we want to put this in the code-style guide we should imho at-least get our own house in order, going “do as we say not as we do” in codereviews is not something i’d like to see us do.
FWIW, the modern c++ core guidelines suggest to not do explicit comparisons against nullptr:
CppCoreGuidelines
A nice side benefit of not writing out things like nullptr is this inevitable situation that would take place during a code review:
But this is but a minor situation. Whatever the decision, just write it down.
We’ll bulk update to nullptr later […] NULL is still everywhere
You mean to say commits like this don’t happen?
IMHO the most straightforward, easiest to read, and less error-prone style is…
if(pointer)
That, by itself, is a full boolean and anything extra is redundant. If we are considering if(pointer != NULL) as better, then why isn’t the following even better: if((pointer == NULL) == false)
Any added complication not only adds more chances for mistakes, as illustrated by LazyDodo, but also adds verbosity. The line will be longer and therefore take longer to grasp. And will also be more likely to wrap, making it even harder to read.
Redundancy in booleans is just redundant.
That’s interesting. It’s a shame that they don’t provide any motivation.
That’s not what I’m suggesting to do, and you know this is ridiculous.
Shorter lines are not by definition easier to grasp, I’m sure you have seen plenty of counter-examples.
The accidental assignment is something to be careful with, for sure. Fortunately, my compiler (gcc) warns about such things:
…/blender/source/blender/blenkernel/intern/constraint.c:4888:7: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
4888 | if (object = NULL) {
| ^~~~~~
So far all the counter-arguments are about the comparison itself. However, nobody talks about what I think is the more important aspect: the ease of finding the NULL-check when browsing through complex code. When the code gets more complex, I really don’t mind the explicit NULL check so that it stands out and is easily found.
Furthermore, the explicit check also shows that the variable in question is indeed a pointer. With if (x), the variable x can be of pretty much any type. Due to the effects of older C standards, and some very long functions, there can be a significant distance between a variable declaration and its use.
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
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.
That’s my point exactly. Code is read so much more than it’s written, readability is paramount.
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.
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.
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.
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 (!).
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.
As Sybren noted, for him the purpose of explicit checks is the following question:
Can this pointer be
NULLat 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:
& reference (if copies are not a reasonable option).non_null template if applicable.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.
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.
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).
Has there been any decision in the meantime or should we just go with the style used in the existing code when writing patches?
Go with the surrounding code style for now. I think there are good arguments for either approach. To recap:
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.