<div class="gmail_quote">On Tue, May 29, 2012 at 7:33 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="h5"><br><div><div>On May 23, 2012, at 3:04 PM, Richard Trieu wrote:</div><br><blockquote type="cite"><div class="gmail_quote">On Mon, May 21, 2012 at 7:01 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On Wed, May 16, 2012 at 11:42 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
On May 15, 2012, at 4:10 PM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
<br>
> 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:<br>
><br>
> enum A {<br>
> FIRST = 1,<br>
> SECOND = 1<br>
> };<br>
<br>
</div></div>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.<br></blockquote><div><br></div></div>
<div>
Bugs of this type would be difficult to discover. In general, most testing is positive testing, such as:</div><div><br></div><div>A test = FIRST:</div><div>assert(test == FIRST);</div><div><br></div><div>Less common is negative testing:</div>
<div><br></div><div>A test = FIRST;</div><div>assert(test != SECOND);</div><div><br></div><div>Also, narrowing the scope of the warning to just enum values created from literal values should make this a more precise warning.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
As for the actual patch…<br>
<br>
+def warn_identical_enum_values : Warning<<br>
+ "all elements of %select{anonymous enum|%1}0 have value %2">,<br>
+ InGroup<DiagGroup<"unique-enum">>, DefaultIgnore;<br>
+<br>
<br>
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.<br>
<br></blockquote></div><div>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.</div><div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // Keep track of whther all the elements have the same value.<br>
+ bool AllElementsEqual = true;<br>
+ llvm::APSInt LastVal;<br>
<br>
Typo "whther".<br>
<div><br>
+ // Keep track of whether every enum element is the same value.<br>
+ if (AllElementsEqual && i > 0) {<br>
+ if (InitVal.getBitWidth() > LastVal.getBitWidth())<br>
+ AllElementsEqual = InitVal == LastVal.extend(InitVal.getBitWidth());<br>
+ else if (InitVal.getBitWidth() < LastVal.getBitWidth())<br>
+ AllElementsEqual = InitVal.extend(LastVal.getBitWidth()) == LastVal;<br>
+ else<br>
+ AllElementsEqual = InitVal == LastVal;<br>
+ }<br>
</div>+ LastVal = InitVal;<br>
+<br>
<br>
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.<br>
<br>
- Doug<br>
<br>
</blockquote></div></div><br></blockquote><div>New patch. </div></div>Moved the unique enum check after all the enum initialization.<div>Removed DefaultIgnore from the warning.</div><div>Only warn when all elements are set with an integer or boolean literal.</div>
</blockquote><br></div></div></div><div><div>LGTM.</div><div><br></div></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><br></div></blockquote></div>Committed r157666