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)
if (!mmd_dispatch_i_pp(INTERP, p1, p2, MMD_EQ))
return 0;
and it's also potentially the case that !mmd_dispatch_i_pp(INTERP, p1,
p2, MMD_EQ) evaluates to false, hence the final return of the function
could be executed.
The question now becomes: what is Coverity Prevent trying to tell us?
Is it getting mixed up somehow?
Any help would be greatly appreciated!
Paul
