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.
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.
Josh
