Re: Yoda style conditions (was: Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c)

[Guy Harris <guy () alum mit edu>]

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.
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe