This is an archive of the discontinued LLVM Phabricator instance.

Extend -Wtautological-overlap-compare to accept negative values and integer conversions
ClosedPublic

Authored by rtrieu on Aug 9 2019, 6:54 PM.

Details

Summary

-Wtautological-overlap-compare checks compound conditionals which compare the same value multiple times against a constant. This patch allows the constant to be a negative value or implicitly cast from another integer type. There's also a minor change to reachable code analysis so that the negative value can act like other values for purposes of silencing the warning.

Diff Detail

Repository
rL LLVM

Event Timeline

rtrieu created this revision.Aug 9 2019, 6:54 PM
NoQ added a reviewer: NoQ.Aug 9 2019, 8:18 PM

Good improvement, thanks

jfb added a subscriber: jfb.Aug 10 2019, 8:59 AM
jfb added inline comments.
test/Sema/warn-unreachable.c
437 ↗(On Diff #214481)

Why this change?

rtrieu marked an inline comment as done.Aug 12 2019, 2:39 PM
rtrieu added inline comments.
test/Sema/warn-unreachable.c
437 ↗(On Diff #214481)
​  if (y == -1 && y != -1)  // condition from warn-unreachable.cpp
  if (- 1)  // condition here

The test case below in warn-unreachable.cpp all have silence notes on them. For consistency, I wanted the new case involving (y == -1 && y != -1) to also have a silence case. The change to the reachability analysis to do that would also generate a silence note for (- 1). Since (- 1) now generates a silence, I picked a different unary operator that would not.

NoQ accepted this revision.Aug 12 2019, 3:08 PM

Looks great, thanks! I'd appreciate direct CFG tests for the part that changes the CFG (cf. test/Analysis/cfg.cpp), but i don't insist. Let's make sure @jfb is happy and commit :)

lib/Analysis/CFG.cpp
1104 ↗(On Diff #214481)

Typo: "subexpressions".

This revision is now accepted and ready to land.Aug 12 2019, 3:08 PM
jfb added a comment.Aug 12 2019, 4:10 PM

I haven't reviewed in depth, but generally this looks good.

rtrieu updated this revision to Diff 215021.Aug 13 2019, 8:07 PM
rtrieu marked an inline comment as done.
In D66044#1626008, @NoQ wrote:

Looks great, thanks! I'd appreciate direct CFG tests for the part that changes the CFG (cf. test/Analysis/cfg.cpp), but i don't insist. Let's make sure @jfb is happy and commit :)

I added a test case to show the new unreachable node in the CFG.

NoQ accepted this revision.Aug 16 2019, 4:42 PM

*appreciates CFG tests*

test/Analysis/cfg.cpp
504–506 ↗(On Diff #215021)

Looks like runaway formatting.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 7:37 PM