[cfe-dev] Possibly invalid enum tautology warning

[David Blaikie]

On Sun, Aug 7, 2011 at 4:16 PM, Peter Geoghegan <peter at 2ndquadrant.com> wrote:
> On 7 August 2011 22:21, John McCall <rjmccall at apple.com> wrote:
>> On Aug 7, 2011, at 1:32 PM, David Blaikie wrote:
>>> On Sun, Aug 7, 2011 at 12:29 PM, John McCall <rjmccall at apple.com> wrote:
>>>> but it sounds
>>>> like you're arguing that we should suppress warnings when another
>>>> platform exists where the warning wouldn't trigger.
>>>
>>> In this particular case I think that's what he's saying, yes. But I
>>> wouldn't say that implies the general statement you've made.
>
> That isn't what I meant at all.

Ok, I was assuming that by "when another platform exists where the
warning wouldn't trigger" he means a sufficiently what you're saying
below, not simply "when some less intelligent compiler doesn't warn,
we wouldn't warn" which it sort of sounds like how you're reading
that.

(yay, language - and you're both welcome to tell me to stop putting
words in each other's mouths. Just trying to make some kind of dialog
here rather than watching things go past one another as they seem to
sometimes)

> I don't like tautologies any more than
> you guys. What I meant was that there could in principle be a compiler
> informed by exactly the same concerns as Clang and exactly as mature,
> that differed only in that it invariably made enums signed, which is a
> very small difference that I believe is allowed for by the standard.
> That compiler wouldn't show this warning though, because it wouldn't
> be tautological there merely because, as it happens, enums are never
> unsigned there. /The tautology is not essential to the code/.
>
>> He phrased it pretty generally.  I was shying away from the specifics
>> of this case because, while the underlying type of an enum is
>> indeed implementation-specific, the promotion rules actually are
>> governed by the standard, and therefore the behavior of this
>> promotion is not really compiler-specific at all.  This would be
>> clearer if we saw the actual enum definition.
>
> Are you sure that the promotion rules are governed by the standard?

The promotion rules about what kind of comparison should be done when
you do "my_enum < 0" - what type the operands should be promoted to,
etc. Assuming that's what John's talking about, I agree - they are
governed by the standard, though I'm with Peter in that I'm not quite
sure how they apply here.

Given that we're talking about a warning, not an error, we're all
operating on the assumption that the code is valid/correct C++, but
just talking about what to

> I attached my example, but I guess that isn't supported on this list.

Nope, my fault - it was attached, I just didn't see it.

> I'm simply building with clang (not clang++) from SVN tip.

Ah, C, ok. Yep, I see this as well when compiling as C. Curious that
it's not consistent between C and C++.

>>>> it's
>>>> contrary to what most people writing portable code want;  indeed,
>>>> we frequently get the dual request, to enable warnings on code that
>>>> works on the current platform but wouldn't on something else.
>>>
>>> & what's your opinion when such a request is made?
>>
>> That it's an interesting idea, but extremely false-positive prone, and it
>> would be expensive to get most of those false positives out.  Very few
>> people are interested in truly arbitrary portability — I mean, case in
>> point, our code base consistently assumes that 'unsigned' is at least
>> 32 bits (the host unsigned, not the target unsigned, of course).  That's
>> not a perfectly portable assumption at all, but it's true of every
>> platform we will ever care about running on.  There's no good way
>> to deduce that kind of thing.

Sure enough - just a matter of whether it coincidentally lines up with
some feature (such as this warning) we're providing. In this case
we're actually warning about a perfectly reasonable (from the C++
standards point of view) piece of code. I think this is more
significant than not warning about some construct that's non-portable.
At least in the latter case we're not pushing people away from writing
portable code - we're just not pushing them towards it.

My side question is, now that Peter's provided the repro: Why do we
see this when compiling C code, but not when compiling C++? (I guess
perhaps due to the promotion issues previously mentioned? that C
parses it as int < int rather than C++ which parses it as enum < int?)
Should we be warning in the C++ case too?

> Maybe so. I'm not sure that applies in this case, because I'm not sure
> if some /relevant/ compiler exists that actually never has unsigned
> enums. It's certainly possible, as I've said. I certainly wouldn't
> advocate spending a lot of time on it. I just thought that it was
> worth drawing the community's attention to this issue.

Yeah, more of an interesting philosophical question than an
important/immediately practical one. I'm still having a hard time
wrapping my head around the actual scenario where you hit this,
though.

>> It's hard to get too excited about adding
>> extensive portability checking when any sane setup will have a
>> buildbot which warns about the problem within a couple of hours.
>
> The issue here is that a warning is seen which is valid for Clang, but
> may not necessarily be valid on other platforms, entirely because of a
> reasonable design decision that the compiler author made that is
> totally unrelated to the quality of the code that is being compiled. A
> buildbot isn't going to help here.

I think the point is that the only practical reason to be compatible
about the warnings is to provide portability, and the only real way to
write portable code is to compile it on all the compilers you want to
be portable to. Buildbots will keep you in check & make sure your
source still compiles on all those compilers. So when you meet the
compiler that uses signed values for unspecified enum elements you'll
find that your test cases break because you were assuming the integer
values of those enumerants would be positive - so you add the check,
get clang's warning & suppress it because you know you need it on some
other compiler even if you don't need it on clang.

But yes, if it's cheap/easy to do, I'd think erring on the side of
"don't warn if there exists some other compliant implementation for
which the warning wouldn't be valid" is of some value & is more
important (though not necessarily very important) than providing
warnings in clang that aren't true of clang but are true of other
(possibly hypothetical) implementations (the scenario John mentioned).

- David