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

[Richard Trieu]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120523/44c1d9ae/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unique-enum2.patch
Type: application/octet-stream
Size: 3625 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120523/44c1d9ae/attachment.obj>