-Wunique-enum; is it worth it?

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?

One problem I see right now is that it doesn't handle enums that represent bit masks, e.g.:

  MaskA = 0x1,
  MaskB = 0x1

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?

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.

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:

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

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.

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?

Not to hijack your thread, and I know it's just one example.... but catching 'if' conditions that match previous ones would be valuable whether there are enums involved or not. I filed a PR long ago:
<http://llvm.org/bugs/show_bug.cgi?id=9193>

This is something PVS Studio can catch! An example I saw the other day:

This warning caught around 5 bugs. Roughly twice that amount in false positives needed to be silenced.

One way that this may manifest is during
testing of enum values:

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

Not to hijack your thread, and I know it’s just one example… but catching ‘if’ conditions that match previous ones would be valuable whether there are enums involved or not. I filed a PR long ago:
<http://llvm.org/bugs/show_bug.cgi?id=9193>

This is something PVS Studio can catch! An example I saw the other day:

Does PVS Studio only catch identical conditional (x == 5 and later x == 5) or can it catch equivalent, but differently coded conditionals (x == 5 then x == 2 + 3)?

Please discuss this on another thread by changing the subject line at least.

Thanks Richard.

IMO, that’s a pretty dreadful signal-to-noise ratio. That’s a 66% false positive rate. That’s the kind of false positive rate I’d consider a warning being DefaultIgnore, not be on by default. Do the other false positives look like cases the warning logic could be improved to handle? Less than 10% false positive rate seems required to me for this warning to be on by default.

I have no problems with switching this to DefaultIgnore. Looking back, through the false positives, it looks like the most common case would be when the enum is simply there to hold a collection of constants. I think checking if there is a variable of the enum type would be the best way to cut down on false positives, but I am not sure how to go about implementing that.

I don’t think this warning belongs in Clang at all, even under DefaultIgnore. The false positive rate is far too high for it to be useful for the majority of users, and it isn’t the kind of warning that maps well to DefaultIgnore, because it doesn’t correspond to some stylistic decision or usage pattern…. it’s just too noisy for the majority of users.

  • Doug

I agree. This seems highly stylistic and not generally applicable. Unless there are objections, I will remove this warning entirely tomorrow. I think it was a good experiment, but I don’t see the general payoff here.

The false positive rate for this warning in under-development code at
Google is pretty low, one FP in 14 cases in August. Most of the true
positives come from generated code in protocol buffers -- enumerators
in protobufs must all have explicit values, so copy-and-paste leads to
wrongly duplicated values.

-Matt

I don't think this warning belongs in Clang at all, even under
DefaultIgnore. The false positive rate is far too high for it to be useful
for the majority of users, and it isn't the kind of warning that maps well
to DefaultIgnore, because it doesn't correspond to some stylistic decision
or usage pattern…. it's just too noisy for the majority of users.

I agree. This seems highly stylistic and not generally applicable. Unless
there are objections, I will remove this warning entirely tomorrow. I think
it was a good experiment, but I don't see the general payoff here.

The false positive rate for this warning in under-development code at
Google is pretty low, one FP in 14 cases in August.

This interesting, but the warning still seems a bit too stylistic, and not generally useful.

Most of the true
positives come from generated code in protocol buffers -- enumerators
in protobufs must all have explicit values, so copy-and-paste leads to
wrongly duplicated values.

It seems to me that the checking should then be done when the protocol buffer code is generated, catching the problem very early, rather then an enforcing a style that doesn't have widespread applicability.