Maybe you could add a fixme that proposes SECOND = FIRST? That is quite common idiom esp. with LAST_VALUE = SECOND.
-- Erik On 24 mei 2012, at 00:04, Richard Trieu <[email protected]> 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. > <unique-enum2.patch> > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
