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.
unique-enum2.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
