Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Eric Blake]

On 08/01/2014 12:41 AM, Markus Armbruster wrote:

>>>>> +        if (data == NULL) {
>>>>
>>>> Wouldn't it be even more idiomatic as:
>>>>
>>>> if (!data) {
>>>>
>>>> Probably applies throughout your series.
>>>>
>>> OK, will do. Thanks!
>>
>> Not so quick! You are free to use that in your patches, but please don't
>> change all code that way without the author's consent. Just like "equals
>> null" is a natural English way of reading, compared to "null equals
>> something", "not null" reads like a boolean expression to me, and even
>> worse while all valid C, "not strcmp" leads to mind-boggling inverted
>> logic...

If it's going to be controversial, then the right thing to do is that
both '(!ptr)' and '(ptr == NULL)' are acceptable, and that preference
should be given to consistency to nearby code.

> 
> !strcmp() is somewhat error prone, because it suggests inequality.

That's why libvirt uses a STREQ() macro; it evaluates to !strcmp under
the hood, but it's a LOT easier to reason about 'STREQ(a, b)' than it is
to reason about '!strcmp(a, b)'.

> Can't claim that for !data.  That one suggests "no data", which is
> exactly right.  Like Eric, I prefer it to the cumbersome data == NULL.

I also prefer 'if (ptr)' over 'if (ptr != NULL)'.

> data == 0 is right out.

Correct, especially if data is a pointer (hmm, our coding style should
mention that we STRONGLY prefer NULL over 0 when referring to the null
pointer).

> 
> Since there's no consensus on !data vs. data == NULL, you're free to
> follow your own taste in new code.  When changing existing code, imitate
> nearby code.  When nearby code is inconsistent, it's your own taste
> again.

Yes, so capturing this sentiment in the coding style guide would be
worthwhile.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature