Wireshark-dev: Re: [Wireshark-dev] Yoda style conditions (was: Re: [Wireshark-commits] rev 3597
(OT, sorry for my noise on the list)
Hi,
thanks a lot for your explanations and regarding the
non-buggy/buggy analyzers.
I'm sorry to ask again an OT but I'm not sure if I understood
correctly your explanation regarding `if(pointer)'-checks in
general for `mandatory' pointers. Also it could be that there is
no easy-and-always-true answer.
* Guy Harris wrote on Wed, Feb 23, 2011 at 11:42 -0800:
> > (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 meant, I think it could be doubtful to blindly extend some
function using a `mandatory' pointer internally like let's say as
minimal example:
static child_t* getChild(parent_t *parent) { return parent->child; }
to add some if(parent) to make a static code analyzer silent.
Since if(p) can be true for invalid p, the whole if() could be
considered to be removed (replaced by an assert()), because the
caller must ensure a valid p anyway.
Probably I just thought that because I had a wrong impression of
considered changes and the pointers in these cases are `optional'
(i.e. can be NULL).
On the other hand I imagine there are good reasons to do exactly
that (add `if(p)' checks) but I just fail to see these reasons
because of my limited view.
> 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...)
Yes, of course.
What I meant was that probably no good solution would be adding
some `if(p)' inside the function (to make the analyzer think the
pointer is optional and not generate the warning, even if this
logically makes no sense).
Also, I think for NULL pointers this is much more easy than to
check for possibly deleted pointers (or otherwise stale pointers
or copies of pointers realloc'd after the copy etc).
Unfortunately I have no clue how much a good static code analyzer
can help here. Maybe it could spot some of such problems and
assist developing, but if I had to guess I would guess
determination of such conditions would be quite limited.
Or is it considered good to add such if(p)-checks because they
are cheap and could avoid a crash in at least some cases?
> 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.
So this does mean not to use `if(p)' for mandatory pointers (but
maybe instead some __attribute__((nonnull)) and/or a static code
analyzer), did I understand correctly?
[BTW, when using glibc functions that have such __attribute__
it might be needed to maintain those also on own functions
(preprocessor condionally, to keep platform independence). When
using many different compilers, is this getting confusing?]
Or here also, better keep such a check in the hope that some
further processing can still be useful (given that if processing
further leads to an incorrect result, this incorrect result does
not harm too much of course, like in some GUI or log message).
(sorry for the noise on the list and to waste your time, but I'd
like to benefit from the experience)
Best regards,
Steffen
About Ingenico: Ingenico is a leading provider of payment, transaction and business solutions, with over 15 million terminals deployed in more than 125 countries. Over 3,000 employees worldwide support merchants, banks and service providers to optimize and secure their electronic payments solutions, develop their offer of services and increase their point of sales revenue.
http://www.ingenico.com/.
This message may contain confidential and/or privileged information. If you are not the addressee or authorized to receive this for the addressee, you must not use, copy, disclose or take any action based on this message or any information herein. If you have received this message in error, please advise the sender immediately by reply e-mail and delete this message. Thank you for your cooperation.
P Please consider the environment before printing this e-mail
