Page MenuHomePhabricator

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

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

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
​  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

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

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