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

[Douglas Gregor]

On May 23, 2012, at 3:04 PM, Richard Trieu wrote:

> On Mon, May 21, 2012 at 7:01 PM, Richard Trieu <rtrieu at google.com> wrote:
> 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
> 
> 
> New patch. 
> Moved the unique enum check after all the enum initialization.
> Removed DefaultIgnore from the warning.
> Only warn when all elements are set with an integer or boolean literal.

LGTM.

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120529/6099ff29/attachment.html>