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

[Richard Trieu]

On Tue, May 29, 2012 at 7:33 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> 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
>
> Committed r157666
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120529/392a6e24/attachment.html>