On Wed, Sep 12, 2012 at 2:59 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Wed, Sep 12, 2012 at 2:30 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>On Tue, Sep 11, 2012 at 3:26 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>

<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

I've started receiving feedback about -Wunique-enum from some users and I'm wondering if it is worth it.  Can we discuss what was the motivation behind this warning and what kind of bugs it was meant to catch?<br>




<br>
One problem I see right now is that it doesn't handle enums that represent bit masks, e.g.:<br>
<br>
  MaskA = 0x1,<br>
  MaskB = 0x1<br>
<br>
and so on.  There are legitimate cases where declaring a bunch of constants in an enum is okay, and it is fine for them to have the same value.  Has this warning caught any real bugs?<br>
<br>
In general, I think it is fine to give warnings a new try and then evaluate them.  I'm leaning towards removing this warning since I don't see what value it brings, but I wanted to know what the motivation was behind this warning before pressing on doing that.<br>




</blockquote></div><br></div><div><div>The motivating issue is to catch issues that arise when enums have the same value.  Users may assume that enums are unique, leading to making unfounded assumptions about the enum.  One way that this may manifest is during testing of enum values:</div>



<div><br></div><div>enum Foo { A = 0, B = 0 };</div><div>Foo f = B;</div><div>if (f == A)</div><div>  ActOnA();  // Runs since A == B</div><div>else if (f == B)</div><div>  ActOnB();  // Never runs.</div></div><div><br></div>



<div>There were several things added to reduce false positives.  Anonymous enums are ignored.  All the enum constants must be initialized with a literal.  This does not trigger if only some of the enum constants have the same value (i.e. if there's 3 constants with one value, and 2 of another).  A fix-it hint is also included to show to silence this warning.</div>

</blockquote><div><br></div></div></div><div>Richard, I know you also caught a number of very real bugs with this warning. Roughly how many? How many times did you have to silence the warning?</div></div></div>
</blockquote></div><br><div>This warning caught around 5 bugs.  Roughly twice that amount in false positives needed to be silenced.</div>