[cfe-commits] [PATCH] Improve -Wtautological-constant-out-of-range-compare

[Richard Trieu]

On Mon, Nov 12, 2012 at 1:17 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> On Mon, Nov 12, 2012 at 1:04 PM, Richard Trieu <rtrieu at google.com> wrote:
> > On Mon, Nov 12, 2012 at 11:22 AM, Eli Friedman <eli.friedman at gmail.com>
> > wrote:
> >>
> >> Comments below.
> >>
> >> On Mon, Nov 12, 2012 at 9:46 AM, Richard Trieu <rtrieu at google.com>
> wrote:
> >> > Make -Wtautological-constant-out-of-range-compare checking take into
> >> > account types and conversion between types.  The old version merely
> checked
> >> > the bit widths, which allowed failed to catch a few cases, while
> warning on
> >> > other safe comparisons.
> >> >
> >> > http://llvm-reviews.chandlerc.com/D113
> >> >
> >> > Files:
> >> >   test/Analysis/additive-folding.cpp
> >> >   test/SemaCXX/compare.cpp
> >> >   test/SemaCXX/warn-enum-compare.cpp
> >> >   lib/Sema/SemaChecking.cpp
> >>
> >> Index: test/SemaCXX/warn-enum-compare.cpp
> >> ===================================================================
> >> --- test/SemaCXX/warn-enum-compare.cpp
> >> +++ test/SemaCXX/warn-enum-compare.cpp
> >> @@ -39,8 +39,8 @@
> >>    while (b == c);
> >>    while (B1 == name1::B2);
> >>    while (B2 == name2::B1);
> >> -  while (x == AnonAA); // expected-warning {{comparison of constant
> >> 42 with expression of type 'Foo' is always false}}
> >> -  while (AnonBB == y); // expected-warning {{comparison of constant
> >> 45 with expression of type 'Bar' is always false}}
> >> +  while (x == AnonAA);
> >> +  while (AnonBB == y);
> >>    while (AnonAA == AnonAB);
> >>    while (AnonAB == AnonBA);
> >>    while (AnonBB == AnonAA);
> >>
> >> Why are you changing this warning?
> >
> >
> > Because it is valid to do:
> > x = (Foo)42;
> > which makes the condition true.
>
> That makes sense... but on the other hand, it reduces the usefulness
> of this warning for the vast majority of enums which don't play tricks
> like this.  In any case, please separate this into its own patch.
>
> There's been some discussion at Apple (in the context of
> -Wassign-enum) about using some heuristics based on the form of enum
> initializers (e.g. whether the initializers are written in the form "a
> << b") to separate enums into two classes: enums which play tricks
> with bitfields, and enums which enumerate simple values.  Do you have
> an opinion here?
>
> -Eli
>

I think that enum checking should be split off into its own warning.  With
"tautological" in its name, this warning seems to imply a certain
absoluteness to it.  The existence of counter-examples would undermine that.

Splitting enums into two types for better checking sounds like a good idea,
and is probably better than the checks we do now.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121112/7bcb43bb/attachment.html>