Wireshark · Wireshark-dev: Re: [Wireshark-dev] Yoda style conditions (was: Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilt

[Guy Harris <guy@xxxxxxxxxxxx>]

On Feb 23, 2011, at 5:52 AM, Steffen DETTMER wrote:

> (OT)
> 
> * Jeff Morriss wrote on Thu, Feb 17, 2011 at 09:27 -0500:
> 
>> I imagine there are going to be a LOT of "if (a)" checks in 2 million 
>> LOC.  Do we really want to change them all?
> 
> shouldn't the number of such problems reported by the static code
> analyzer match the number of places to be changed?

Yes, and the problem, it turns out, isn't with "if (a)" checks, it's with "if (a && {something else})" checks.  Gerald changed the nmake files to turn off the "dereferencing a null pointer" checks to avoid that particular MSVC++ bug.  (I don't think the Clang static analyzer has this problem.)

> (personally, I think in general it can be quite doubtful to add
> some if (p != NULL) around *p, maybe p was checked before or was
> initilized with some p = &o or whatever.

A non-buggy static analyzer would detect that and not warn about it.  Even the MSVC++ analyzer can handle at least some instances of that.

> I would be afraid that
> such warning make some people to blindly add if (p != NULL) with
> the possible result to make the app misbehave in a way hard to
> find instead of crashing quick, straight-forward, direct and
> clean, which is easy to debug. We even have a developer rule
> usually NOT to use things like `if (p)' or `if (p != NULL)' [for
> mandatory pointers, but assert(p) should be used] because
> pointers cannot be checked; *p also crashes when already freed or
> uninitialized or whatever - comments appreciated :)).

If by "mandatory pointer" you mean "pointer that must not be null" - e.g., if you have a routine that takes a pointer to an XXX and does something with that XXX, so the pointer is expected not to be null - either

	1) the static analyzer will deduce that a pointer is mandatory and will warn, for example, about a call to the routine in question that passes it a null pointer, so the fix should be not to pass a null pointer to it (so that you have an even-easier-to-debug-than-a-crash *build*-time error; run-time errors aren't always that easy to debug...)

or

	2) if it can't or won't do that, the developer should, if the analyzer supports it, decorate the routine with a "this pointer must not be null" argument, to let the static analyzer do the check in question.