Page MenuHomePhabricator

[clang-tidy] Catch trivially true statements like a != 1 || a != 3
ClosedPublic

Authored by watsond on Feb 10 2017, 4:58 PM.

Details

Summary

Catch trivially true statements of the form a != 1 || a != 3. Statements like
these are likely errors. They are particularly easy to miss when handling enums:

enum State {
RUNNING,
STOPPED,
STARTING,
ENDING
}

...
if (state != RUNNING || state != STARTING)
...

Diff Detail

Repository
rL LLVM

Event Timeline

watsond created this revision.Feb 10 2017, 4:58 PM
etienneb edited edge metadata.Feb 13 2017, 8:03 AM

Could you add some tests with enums (like the one in your description)?
This is missing and it's a nice to have.

clang-tidy/misc/RedundantExpressionCheck.cpp
244 ↗(On Diff #88073)

The good news is that this code will be catch by this check!

if (OpcodeLHS == BO_NE || OpcodeLHS == BO_NE)   <<-- redundant expression

Should be:

if (OpcodeLHS == BO_NE || OpcodeRHS == BO_NE)
etienneb added inline comments.Feb 14 2017, 7:35 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
244 ↗(On Diff #88073)

btw, it should be:

if (OpcodeLHS == BO_NE && OpcodeRHS == BO_NE)
alexfh requested changes to this revision.Feb 28 2017, 3:14 AM
This revision now requires changes to proceed.Feb 28 2017, 3:14 AM
watsond marked 2 inline comments as done.Feb 28 2017, 2:52 PM

Could you add some tests with enums (like the one in your description)?
This is missing and it's a nice to have.

Added

clang-tidy/misc/RedundantExpressionCheck.cpp
244 ↗(On Diff #88073)

Thanks for catching!

watsond updated this revision to Diff 90087.Feb 28 2017, 2:53 PM
watsond edited edge metadata.
watsond marked an inline comment as done.

Fixed logic error, added enum test case

alexfh accepted this revision.Mar 2 2017, 12:43 AM

LG unless Etienne has any concerns.

This revision is now accepted and ready to land.Mar 2 2017, 12:43 AM
etienneb accepted this revision.Mar 2 2017, 7:07 AM

thx for the improvement
lgtm

alexfh added a comment.Mar 2 2017, 8:35 AM

Do you need someone to commit the patch for you?

Do you need someone to commit the patch for you?

I think so. This is my only LLVM patch, so I doubt I have permissions. Thanks in advance!

Following up. Was this checked in? Do I need to do anything further?

This revision was automatically updated to reflect the committed changes.

Following up. Was this checked in? Do I need to do anything further?

Committed the patch now. Thanks for the reminder!