Page MenuHomePhabricator

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

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

Details

Reviewers
cfe-commits
NoQ
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.Fri, Aug 9, 6:54 PM
NoQ added a reviewer: NoQ.Fri, Aug 9, 8:18 PM

Good improvement, thanks

jfb added a subscriber: jfb.Sat, Aug 10, 8:59 AM
jfb added inline comments.
test/Sema/warn-unreachable.c
437

Why this change?

rtrieu marked an inline comment as done.Mon, Aug 12, 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.Mon, Aug 12, 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.Mon, Aug 12, 3:08 PM
jfb added a comment.Mon, Aug 12, 4:10 PM

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

rtrieu updated this revision to Diff 215021.Tue, Aug 13, 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.Fri, Aug 16, 4:42 PM

*appreciates CFG tests*

test/Analysis/cfg.cpp
504–506

Looks like runaway formatting.