[cfe-commits] [Patch] Add a warning to catch enums with all elements having the same value

[Richard Trieu]

On Wed, May 16, 2012 at 11:42 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On May 15, 2012, at 4:10 PM, Richard Trieu <rtrieu at google.com> wrote:
>
> > Add -Wunique-enum which will warn on enums with at least 2 elements such
> that all elements are the same value.  This will catch enums such as:
> >
> > enum A {
> >   FIRST = 1,
> >   SECOND = 1
> > };
>
> Why do we need this warning? Personally, I can't recall having seen this
> be the source of any bugs, and it's scope is so narrow that it doesn't seem
> worth adding.
>

Bugs of this type would be difficult to discover.  In general, most testing
is positive testing, such as:

A test = FIRST:
assert(test == FIRST);

Less common is negative testing:

A test = FIRST;
assert(test != SECOND);

Also, narrowing the scope of the warning to just enum values created from
literal values should make this a more precise warning.


>
> As for the actual patch…
>
> +def warn_identical_enum_values : Warning<
> +  "all elements of %select{anonymous enum|%1}0 have value %2">,
> +  InGroup<DiagGroup<"unique-enum">>, DefaultIgnore;
> +
>
> Default-ignore warnings are always a red flag. If this warning is
> important, it should be on by default. If it's not important, we shouldn't
> add it.
>
> I can certainly have this on by default if that's your preference.  The
change to also check for literal values should greatly reduce the number of
false positives.


> +  // Keep track of whther all the elements have the same value.
> +  bool AllElementsEqual = true;
> +  llvm::APSInt LastVal;
>
> Typo "whther".
>
> +    // Keep track of whether every enum element is the same value.
> +    if (AllElementsEqual && i > 0) {
> +      if (InitVal.getBitWidth() > LastVal.getBitWidth())
> +        AllElementsEqual = InitVal ==
> LastVal.extend(InitVal.getBitWidth());
> +      else if (InitVal.getBitWidth() < LastVal.getBitWidth())
> +        AllElementsEqual = InitVal.extend(LastVal.getBitWidth()) ==
> LastVal;
> +      else
> +        AllElementsEqual = InitVal == LastVal;
> +    }
> +    LastVal = InitVal;
> +
>
> If you did this in the loop below, where we adjust the initializer values
> to all have the same size/# of bits, it would be easier.
>
>        - Doug
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120521/2c7cdb11/attachment.html>