On Tue, May 29, 2012 at 7:33 AM, Douglas Gregor <[email protected]> wrote:
> > On May 23, 2012, at 3:04 PM, Richard Trieu wrote: > > On Mon, May 21, 2012 at 7:01 PM, Richard Trieu <[email protected]> wrote: > >> On Wed, May 16, 2012 at 11:42 AM, Douglas Gregor <[email protected]>wrote: >> >>> >>> On May 15, 2012, at 4:10 PM, Richard Trieu <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
