Wireshark mailing list archives
Re: Yoda style conditions (was: Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c)
From: "Steffen DETTMER" <Steffen.DETTMER () ingenico com>
Date: Thu, 24 Feb 2011 21:00:43 +0100
(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 ___________________________________________________________________________ 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
Current thread:
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Stig Bjørlykke (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Guy Harris (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Guy Harris (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Guy Harris (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Gerald Combs (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Guy Harris (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Guy Harris (Feb 17)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Jeff Morriss (Feb 17)
- Yoda style conditions (was: Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c) Steffen DETTMER (Feb 23)
- Re: Yoda style conditions (was: Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c) Guy Harris (Feb 23)
- Re: Yoda style conditions (was: Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c) Steffen DETTMER (Feb 24)
- Re: Yoda style conditions Jeff Morriss (Feb 24)
- Re: [Wireshark-commits] rev 35975: /trunk/epan/dfilter/ /trunk/epan/dfilter/: dfilter-macro.c dfilter.c Guy Harris (Feb 17)