D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 5540 - Probable bug-hiding redundancies
Summary: Probable bug-hiding redundancies
Status: RESOLVED MOVED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P2 enhancement
Assignee: No Owner
URL:
Keywords: bootcamp
Depends on:
Blocks:
 
Reported: 2011-02-07 12:33 UTC by bearophile_hugs
Modified: 2022-08-15 14:09 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description bearophile_hugs 2011-02-07 12:33:29 UTC
This is a real bug in good C++ code:

if ((p_newHeader.frame_num != m_lastSlice.frame_num) ||
    (p_newHeader.pic_parameter_set_id !=
     p_newHeader.pic_parameter_set_id) ||
    (p_newHeader.field_pic_flag != p_newHeader.field_pic_flag) ||
    (p_newHeader.bottom_field_flag != m_lastSlice.bottom_field_flag)
    ){
    return false;
}


The bug is of the kind:
if (x != x) {}

Two other similar bugs:
if (x == x) {}

auto y = x - x;


For the compiler it's easy to spot such three situations. They are correct code, yet experience shows that often such redundancies hide bugs. So I suggest to add to DMD warnings for such tree situations.

See also bug 3878
Comment 1 bearophile_hugs 2011-02-07 15:05:52 UTC
Another case, in good C++ code (probably caused by not complete copy & paste & modify):

return Contains(Sphere(lss.mP0, lss.mRadius)) && 
       Contains(Sphere(lss.mP0, lss.mRadius));

That is of this kind:
x && x
Comment 2 bearophile_hugs 2011-04-07 15:08:11 UTC
Reference:
http://www.viva64.com/en/a/0068/
Comment 3 bearophile_hugs 2011-06-19 16:38:35 UTC
See a very nice example of bug of this kind, that's easy to find for a compiler or lint:

http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=139021
Comment 4 bearophile_hugs 2011-06-19 16:44:30 UTC
(In reply to comment #3)
> See a very nice example of bug of this kind, that's easy to find for a compiler
> or lint:
> 
> http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=139021

The bug in Andrej Mitrovic code was:

if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof != DRUM.sizeof))
{
    mmioClose(hmmio, 0);
    return szErrorCannotRead;
}
Comment 5 bearophile_hugs 2011-06-29 13:24:48 UTC
See also:
http://www.viva64.com/en/b/0103/
http://www.viva64.com/en/d/0090/
Comment 6 bearophile_hugs 2011-06-29 16:56:41 UTC
A class of bug-hiding redundancy:

if (x == 10)
    foo1();
else if (x == 20)
    foo2();
else if (x == 10) // ***
    foo3();


Another class, two assignments to the same variable in a row:

x = foo();
x = bar();


While this is OK:

x = 1;
x = x + 1;
x = foo(x);


Another class (the two branches are equal):

if (x)
    foo();
else
    foo();
Comment 7 Infiltrator 2015-12-09 12:17:38 UTC
I think that you're being a little too zealous.  For instance, foo() in your last example, and any function in general, may not necessarily be pure (in the mathematical sense) so they may produce different results and/or side-effects.
Comment 8 Dmitry Olshansky 2018-05-22 09:45:01 UTC
I think indeed for expressions that are trivially true (or false) there must be an error in the context of || and &&. x == x and x != x is a popular mistake.

Except for true/false literals as they are often used for debugging and disabling parts of program explicitly. It's a balancing act but a lot of vlaue to be had.

On the other hand I'd really just give this to dscanner or simillar lint. I tried dscanner right now but it doesn't seem to pick it up.
Comment 9 RazvanN 2022-08-15 14:09:37 UTC
Walter is against adding warnings, so that path is a dead end.

These checks should be done by a linter that uses dmd as a library.