On 08/10/2007, Joshua Juran <[EMAIL PROTECTED]> wrote: > On Oct 8, 2007, at 4:36 AM, Paul Cochrane wrote: > > > On 08/10/2007, Joshua Juran <[EMAIL PROTECTED]> wrote: > >> On Oct 7, 2007, at 12:43 PM, Paul Cochrane (via RT) wrote: > >> > >>> # New Ticket Created by Paul Cochrane > >>> # Please include the string: [perl #46223] > >>> # in the subject line of all future correspondence about this issue. > >>> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=46223 > > >>> > >>> > >>> Coverity Prevent mentions that the code after the C<if (p1 || p2)> > >>> comparison in src/pmc/pair.pmc can never be executed. Here is the > >>> offending chunk of code (the comments are mine): > >>> > >>> p1 = PMC_pmc_val(SELF); > >>> p2 = PMC_pmc_val(value); > >>> > >>> if (!p1 && !p2) > >>> return 1; /* when p1 = p2 = null, or p1 = p2 = non-null */ > >>> > >>> if (p1 || p2) > >>> return 0; /* when p1 = null, p2 = non-null, or p1 = non-null, > >>> p2 = null */ > >>> > >>> /* which handles all permutations of p1 and p2 */ > >>> > >>> /* hence, the following code can never be executed */ > >>> if (!mmd_dispatch_i_pp(INTERP, p1, p2, MMD_EQ)) > >>> return 0; > >>> > >>> return 1; > >>> > >>> The attached patch removes the dead code, and 'make test' > >>> passes. If > >>> there are no complaints, I'll apply this patch in about 3 days; > >>> if it > >>> is approved, as soon as I am able. > >> > >> The "when ..." comments don't match the code. Depending on which is > >> correct I'd write either > >> > >> return p1 == null && p2 == null; > >> > >> or > >> > >> return (p1 == null) == (p2 == null); > > > > Another good reason why I submitted a patch instead of just committing > > the change :-) On looking at this again I think my reading of the > > code was incorrect. Here's another go: > > > > This path will be taken only if p1 *and* p2 are null. > > if (!p1 && !p2) > > return 1; > > > > This path will be taken if either p1 is non-null or p2 is non-null > > if (p1 || p2) > > return 0; > > > > So the only case left is p1 is non-null *and* p2 is non-null, which > > means this code *is* executed (at some stage) > > False. "Either or" in logic indicates exclusive union, but in > general English does not. Regardless, the test (p1 || p2) includes > the case of both p1 and p2 being non-null, so whatever the state of > p1 and p2, one of those two tests will catch it and return.
You're right. I'm obviously in the middle of a brain-fade of some description... I even checked my thoughts on paper, rather than doing the logic in my head. Oh well, good I posted this to the list.... > > The question now becomes: what is Coverity Prevent trying to tell us? > > Is it getting mixed up somehow? > > Nope. Coverity is right on the money. So, the patch is right (however, for my wrong reasoning)? Is everyone happy if I apply it then? Paul
