Possibly invalid enum tautology warning

Clang Frontend
1

Consider the attached testcase. When built with Clang from SVN tip, it
produces the following warning:

clang_testcase.c:21:11: warning: comparison of unsigned enum expression < 0 is
      always false [-Wtautological-compare]
        if (test < 0)
            ~~~~ ^ ~

Clang has made the enum in the testcase unsigned. This is perfectly
reasonable - the C standard gives leeway as to how the enum can be
represented, so both GCC and Clang will make the representation
unsigned unless they're forced to make it signed by the fact that the
definition explicitly sets constants to negative integer values.

What's also true is that compilers may opt to invariably make enums
signed. While I'm not aware that any compiler does so, I'm also not
aware that they all don't, and I sincerely doubt that the optimisation
GCC and Clang perform here to maximize the number of values that can
be represented is all that useful in practice, so I can certainly
imagine compiler vendors not bothering with it.

Anyway, the above test could be perfectly valid in the case where some
compiler's enum representation is signed. That isn't true of Clang or
GCC, but I feel that Clang should aim to both follow the standard to
the letter and follow it in spirit, allowing people to write portable
code. If you find this scenario implausible, consider that this
actually happened on Postgres, due to the fact that we checked that a
certain enum variable's value could be safely used as an array
subscript within our client library, libpq. We weren't about to trust
arbitrary client code to not pass an invalid value.

I am aware that using the constant literal in place of 0 prevents the
warning from being seen. Nonetheless, I thought that it was worth
considering if this is a case that we could handle better.

Incidentally, Clang now builds Postgres without any warnings, other
than a warning that we get with GCC too and can't really do anything
about. I've blogged about it. Impressively, Clang caught a bug that,
if a certain struct had been used a certain way that we'd generally
consider to be perfectly reasonable, would have resulted in undefined
behaviour. Luckily or unluckily, it actually wasn't being used that
way, but that could have easily changed over time.

clang_testcase.c (320 Bytes)

2

Consider the attached testcase. When built with Clang from SVN tip, it
produces the following warning:

clang_testcase.c:21:11: warning: comparison of unsigned enum expression < 0 is
always false [-Wtautological-compare]
if (test < 0)
~~~~ ^ ~

Could you provide the full source & command line used? My attempt to
reproduce this don't seem to be consistent with your claim. I'm only
getting that warning if I explicitly specify the backing type of the
enum as unsigned:

dblaikie@mediabox:~/Development/scratch$ cat test.cpp
enum foo { X, Y };

int bar(foo f) {
  if (f < 0)
    return 3;
  return 7;
}

int main() {
}
dblaikie@mediabox:~/Development/scratch$ clang++ -Wtautological-compare test.cpp
dblaikie@mediabox:~/Development/scratch$ clang++ -std=c++0x
-Wtautological-compare test.cpp
dblaikie@mediabox:~/Development/scratch$ cat test2.cpp
enum foo: unsigned { X, Y };

int bar(foo f) {
  if (f < 0)
    return 3;
  return 7;
}

int main() {
}
dblaikie@mediabox:~/Development/scratch$ clang++ -std=c++0x
-Wtautological-compare test2.cpp
test2.cpp:4:9: warning: comparison of unsigned enum expression < 0 is
always false [-Wtautological-compare]
  if (f < 0)
      ~ ^ ~
1 warning generated.

What's also true is that compilers may opt to invariably make enums
signed. While I'm not aware that any compiler does so, I'm also not
aware that they all don't, and I sincerely doubt that the optimisation
GCC and Clang perform here to maximize the number of values that can
be represented is all that useful in practice, so I can certainly
imagine compiler vendors not bothering with it.

I don't think this is necessarily an optimization, as such - either
way the compiler would have to have be able to flip between different
representations based on the explicit enum values specified (eg: if
you specify -1 as an enum value it's going to have to used a signed
type, yet if you used the max unsigned nit value as a value in another
enum it'd have to be able to choose unsigned int (or signed long - in
which case apply the same problem to max unsigned long long, etc). The
'optimization' is then just a question of which type it uses by
default - if your values are all within the range of unsigned int and
int, either is equally valid & there's no cost (that I know of) to
choosing one over the other. So not really an optimization)

Anyway, the above test could be perfectly valid in the case where some
compiler's enum representation is signed.

I haven't checked the spec but I'll take your word for this. Seems
possible/reasonable.

That isn't true of Clang or
GCC, but I feel that Clang should aim to both follow the standard to
the letter and follow it in spirit, allowing people to write portable
code.

At which point I sort of question the purpose of the code testing for
enum > 0, really. (details to follow)

If you find this scenario implausible, consider that this
actually happened on Postgres, due to the fact that we checked that a
certain enum variable's value could be safely used as an array
subscript within our client library, libpq.

Could you point to some source - I'm curious about what exactly is
going on here. Are you checking a specific enum value passed to your
API? If so it could be outside the range of actual enum values anyway,
couldn't it? & knowing that the one enum value they passed is within
the range of the array doesn't necessarily tell you anything about
whether the entire range of enums is within the range, does it? So
what do you do with one enum value within the range, but others not?

We weren't about to trust
arbitrary client code to not pass an invalid value.

Shouldn't you be validating this against the range of enum values (eg:
assert(first_enum <= val && val <= last_enum)) ?

I am aware that using the constant literal in place of 0 prevents the
warning from being seen.

By constant literal you mean the first enum value, rather than the
literal 0? That seems like a different check, semantically.

If you want to support this scenario it sounds like there's two steps:

1) validating that a user specified enum value is within the range of
acceptable values (assuming the spec requires that unspecified enum
values are at least contiguous, I haven't checked that detail myself)
- as above

2) validating that the values of an enum without a specified backing
type are usable as array indecies - wouldn't this be better achieved
simply by taking "value - first_enum"? That way it would work no
matter what range the compiler decided to use for your enum values.
(if the C standard requires that unspecified enum values must start
from 0 then this step is unnecessary)

Nonetheless, I thought that it was worth
considering if this is a case that we could handle better.

My best guess, based on experiment, is that it's already handling this
better but I'm not sure it's even necessary. I imagine it must be
being handled deliberately perhaps for the reasons you cited, though
I'm not sure I agree with them as being necessary.

Incidentally, Clang now builds Postgres without any warnings, other
than a warning that we get with GCC too and can't really do anything
about. I've blogged about it. Impressively, Clang caught a bug that,
if a certain struct had been used a certain way that we'd generally
consider to be perfectly reasonable, would have resulted in undefined
behaviour. Luckily or unluckily, it actually wasn't being used that
way, but that could have easily changed over time.

Did you blog about this particular issue? I'd be curious to see what
construct it found.

- David

3

I'm not sure what's actually going on in your example, but it sounds
like you're arguing that we should suppress warnings when another
platform exists where the warning wouldn't trigger. Not only would
that be a ridiculously work-intensive standard to maintain, but 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.

John.

4

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.

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?

I think the key here is that it depends on the semantics of the
warning. I'd say these requests you mention are consistent with
Peter's request: Help me write portable code. If you're warning me
about something that is useful on some implementations & trivial on
others, that's not helping me. If you aren't warning me about
something because it happens to be reasonable on this compiler but
wouldn't be on others, that's not helping (not as much as it could)
either.

Though, yes, I agree that it's probably a lot of work to try to aim
for such a bar - I don't know whether that means clang should throw
out the goal entirely, or just try to do it opportunistically.

I'm sure I'm not sufficiently expressing myself here, but hopefully
it'll make some sense,

- David

5

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.

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.

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.

Plus, it's not like portability is actually assured just by stamping out
portability-related compiler warnings; while an automated checker
can catch some obvious things, portability is actually a long tail
of problems, and projects that actually care about portability
should be regularly building and testing on all their supported
platforms anyway. 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.

I think a good portability checker might make an interesting
static analysis, though.

John.

6

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. 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?
Where, exactly? This is from C99:

"Each enumerated type shall be compatible with char, a signed integer
type, or an
unsigned integer type. The choice of type is
implementation-defined,110) but shall be
capable of representing the values of all the members of the enumeration."

It seems to me that C99 isn't encouraging or requiring this sort of
promotion at all. Maybe this has been widely interpreted to mean "you
should promote". I don't know - I'm not that much of a language
lawyer. There doesn't seem to be anything in this passage to suggests
that promotion should or must happen though.

I attached my example, but I guess that isn't supported on this list.
Here it is again:

#include <stdio.h>

typedef
enum tester
{
  test_constant = 0,
  other_test_constant
} tester;

void show_tautology(tester test);

int main()
{
  tester test = test_constant;
  show_tautology(test);
  return 0;
}

void show_tautology(tester test)
{
  if (test < 0)
    printf("is the above test necessarily tautological?");
}

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

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.

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.

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 a good portability checker might make an interesting
static analysis, though.

Sure.

7

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

8

I'm actually of the opinion that this IS tautological.

enum tester { A = 0, B };

void show_tautology(enum tester test)
{
  if (test < 0)
    printf("is the above test necessarily tautological?");
}

By typing 'test' as a 'tester', you've implicitly promised the compiler that it will only take on values in the enumeration, i.e. A (0) or B (1). It doesn't actually matter what the representation is; these values are perfectly constrained by the C standard.

That is, I would argue that these are tautological as well:

enum foo { bar = -1, baz = -2 };
void testFoo (enum foo test)
{
  if (test < -1)
    printf("unreachable");
  if (test > 5)
    printf("unreachable");
}

This, on the other hand, would not be tautological:

enum { A = 0; }
void test(int i)
{
  if (i < A)
    printf("not tautological");
}

While promotion rules make all of these into int-int comparisons (either signed or unsigned), it seems that the warning is still correct, even if it shows up for the wrong reason. (I agree that we shouldn't really be warning just based on the signedness OR size of the enum.)

Maybe the right solution is to make a new warning for "tautological comparisons with enumeration types", put that under its own flag, and then silence the original warning if an enum type is involved.

Just my two cents,
Jordy

9

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?
Where, exactly? This is from C99:

Ah. It does depend which language we're talking about here.

"Each enumerated type shall be compatible with char, a signed integer
type, or an
unsigned integer type. The choice of type is
implementation-defined,110) but shall be
capable of representing the values of all the members of the enumeration."

C99 6.3.1.8:
  Many operators that expect operands of arithmetic type cause conversions
  and yield result types in a similar way. The purpose is to determine a
  common real type for the operands and result.
  ...
  [If neither operand is long double, double, or float], the integer promotions
  are performed on both operands.
C99 6.3.1.1:
  The following may be used in an expression wherever an int or unsigned
  int may be used:
    — An object or expression with an integer type whose integer conversion
      rank is less than or equal to the rank of int and unsigned int.
    — A bit-field of type _Bool, int, signed int, or unsigned int.
  If an int can represent all values of the original type, the value is converted
  to an int; otherwise, it is converted to an unsigned int. These are called the
  integer promotions.

Clang follows GCC's lead in making unpacked C enums compatible with
unsigned unless they contain signed enumerators; the enum type then
has the integer rank of 'unsigned int' for the purposes of these rules.

So promotion is indeed mandated, but whether it promotes to 'int' or
'unsigned int' is implementation-defined.

The C++ rule is quite different:
  C++0x [conv.prom]p3:
    A prvalue of an unscoped enumeration type whose underlying type
    is not fixed (7.2) can be converted to a prvalue of the first of the following
    types that can represent all the values of the enumeration (i.e., the values
    in the range bmin to bmax as described in 7.2): int, unsigned int, long int,
    unsigned long int, long long int, or unsigned long long int.

I attached my example, but I guess that isn't supported on this list.
Here it is again:

#include <stdio.h>

typedef
enum tester
{
  test_constant = 0,
  other_test_constant
} tester;

void show_tautology(tester test);

int main()
{
  tester test = test_constant;
  show_tautology(test);
  return 0;
}

void show_tautology(tester test)
{
  if (test < 0)
    printf("is the above test necessarily tautological?");
}

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

Okay, yes, in C, when we're emulating the GCC behavior, this
comparison is performed in the type 'unsigned int' and is therefore
tautological.

I still consider this a useful warning, even on enum types. When I
was implementing it, I was persuaded that we ought to have a
special case for comparing against an enumerator which happens
to be zero, because that was indicative of a particular common
and harmless idiom (specifically, things like if (v < v_first || v > v_last)).
I'm loathe to extend that to a direct comparison against zero, where
it really does seem like the programmer is specifically testing for
an error case and might be surprised to realize that their test never
triggers. People who really do want to litter their code with defensive
checks like this should just disable the tautological comparison
check — it's not likely that it would only ever come up with enums.

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.

I do appreciate that.

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.

Well, you're assuming that comparisons like this are always in overly-
defensive code where the tautology is harmless.

John.

10

I still consider this a useful warning, even on enum types. When I
was implementing it, I was persuaded that we ought to have a
special case for comparing against an enumerator which happens
to be zero, because that was indicative of a particular common
and harmless idiom (specifically, things like if (v < v_first || v > v_last)).
I'm loathe to extend that to a direct comparison against zero, where
it really does seem like the programmer is specifically testing for
an error case and might be surprised to realize that their test never
triggers. People who really do want to litter their code with defensive
checks like this should just disable the tautological comparison
check — it's not likely that it would only ever come up with enums.

Respectfully, I have to wonder if that's a practical attitude. They
may not want to "litter" their code. They may just want to do this
exactly once, as for example Postgres was before a patch was committed
that sidestepped the issue. Should they still have to disable the
tautological comparison check?

Well, you're assuming that comparisons like this are always in overly-
defensive code where the tautology is harmless.

No, I'm not. I'm quite simply taking issue with the fact that the
tautology exists only because of a factor that is implementation
defined. /The tautology is not essential to the code/.

11

enum tester { A = 0, B };

Ah, I missed this bit. But I suspect that even without this ("= 0")
you'd get the same behavior - at that point I'd be more inclined to
consider Peter's position, assuming it's possible that a compliant
implementation could use A = -1, B = 0, for example.

If it's guaranteed that unspecified enum values start at zero, and as
has been implied/mentioned, assigning an enum does have UB when
assigned outside the range of enumerated values (much like wedging 2
into a bool produces crazy) then, yes, this warning seems totally fine
to me. It's not complete, of course, but it's not wrong either.

12

enum tester { A = 0, B };

Ah, I missed this bit. But I suspect that even without this ("= 0")
you'd get the same behavior - at that point I'd be more inclined to
consider Peter's position, assuming it's possible that a compliant
implementation could use A = -1, B = 0, for example.

If it's guaranteed that unspecified enum values start at zero,

C99 6.7.2.2p3 specifies "If the first enumerator has no =, the value of its enumeration constant is 0." C++ does the same.

and as
has been implied/mentioned, assigning an enum does have UB when
assigned outside the range of enumerated values (much like wedging 2
into a bool produces crazy) then, yes, this warning seems totally fine
to me. It's not complete, of course, but it's not wrong either.

C99 and C++98/03 say nothing about this. C++0x tried to clean it up a little based on what the committee felt was the intent in C and C++. See C++0x N3290 7.2 [dcl.enum]p7 for the full details, but the relevant conclusion is that, if the smallest enumerator is non-negative, then the values allowed in the enumeration are all non-negative.

  - Doug

[*] whose underlying type is not fixed, as is the case here.

13

C99 6.7.2.2p3 specifies "If the first enumerator has no =, the value of its enumeration constant is 0." C++ does the same.
...
C99 and C++98/03 say nothing about this. C++0x tried to clean it up a little based on what the committee felt was the intent in C and C++. See C++0x N3290 7.2 [dcl.enum]p7 for the full details, but the relevant conclusion is that, if the smallest enumerator is non-negative, then the values allowed in the enumeration are all non-negative.

Thanks for the references, Doug. Yes, that would've surprised me if
storing any non-enum value in an enum was UB - as much as I'd like
that to be the case, it hasn't felt like the language really tried to
make them that safe. (enum classes in C++0x I guess are meant to be
safer, but I haven't read the finer points of those either) But this
guarantee sounds good enough for this case, as you mentioned.

- David

14

I still consider this a useful warning, even on enum types. When I
was implementing it, I was persuaded that we ought to have a
special case for comparing against an enumerator which happens
to be zero, because that was indicative of a particular common
and harmless idiom (specifically, things like if (v < v_first || v > v_last)).
I'm loathe to extend that to a direct comparison against zero, where
it really does seem like the programmer is specifically testing for
an error case and might be surprised to realize that their test never
triggers. People who really do want to litter their code with defensive
checks like this should just disable the tautological comparison
check — it's not likely that it would only ever come up with enums.

Respectfully, I have to wonder if that's a practical attitude. They
may not want to "litter" their code. They may just want to do this
exactly once, as for example Postgres was before a patch was committed
that sidestepped the issue. Should they still have to disable the
tautological comparison check?

There are simple, idiomatic ways to suppress this warning
on a site-by-site basis. You found one yourself. Why is the answer
to diminish the utility of the compiler to other users because Postgres
was doing something unidiomatically?

Well, you're assuming that comparisons like this are always in overly-
defensive code where the tautology is harmless.

No, I'm not. I'm quite simply taking issue with the fact that the
tautology exists only because of a factor that is implementation
defined. /The tautology is not essential to the code/.

We've gone over this before. How do you differentiate this from
anything else arising from something implementation-defined,
like the signedness of wchar_t?

I must also note that we are completely in accord with GCC on the
choice of signedness for this enum; this is not some idiosyncratic
clang thing. This is how it works for probably the majority of your
users.

John.

15

There are simple, idiomatic ways to suppress this warning
on a site-by-site basis. You found one yourself. Why is the answer
to diminish the utility of the compiler to other users because Postgres
was doing something unidiomatically?

It wasn't unidiomatic. I'd say that it was the most natural way of
expressing the problem; the value of the enum had to be used as an
array subscript, and the enum might well have been unsigned for all we
knew - arbitrary third party code is not to be trusted. Since I've
already fixed all warnings when building with Clang that are unique to
Clang, I don't really represent the Postgres community here any more.

I can't see the suppression of this warning as described diminishing
the value of the compiler for everyone; I can see it increasing
Clang's value. That's what I want - otherwise, I wouldn't be spending
my time debating it with you. While this issue may not be of any great
consequence, it is definitely of much greater consequence to Clang
than to Postgres. This warning is absolutely inconsequential to
Postgres. I like Clang too though.

We've gone over this before. How do you differentiate this from
anything else arising from something implementation-defined,
like the signedness of wchar_t?

Perhaps you don't. Is that intended to be a reductio ad absurdum
argument? If so, I don't find the reduction to be absurd, because GCC
doesn't show any tautology warnings there. Have I missed something?

I must also note that we are completely in accord with GCC on the
choice of signedness for this enum; this is not some idiosyncratic
clang thing. This is how it works for probably the majority of your
users.

That's true. The idiosyncratic Clang thing is that Clang complains
about it. I'd be all in favour of that complaining if it was
inherently valid across compiler platforms; at the moment, I am not
convinced of that.

Of course, unlike the last thread I started on Clang's warnings, it's
not as if I feel that strongly about this. Apparently you don't see it
quite as I do; you want the compiler to correct the code while making
assumptions about the platform or compiler in use (assuming that it
will only be the current platform or one like it). I want the compiler
to provide objective diagnostics in a vacuum. I respect these
differences. I just disagree.

Clang is currently being used as a sort of lint tool by many, because
its diagnostics are much better than GCC's. People who use it in this
way are perhaps more likely to be sympathetic to my view - it should
be possible to use a compiler as a lint tool without caring about the
binaries it produces, provided the logical input to the compiler is
the same (which is not to say I consider Clang to be a mere lint
tool). I'm sure you could list corner cases or grey areas here, but
this is just an ideal, and like most ideals it's never going to be
completely realised. The behaviour that I'm suggesting we might want
to consider here though, removing the warning under these
circumstances, could easily be realised.

16

I'd say that it was the most natural way of
expressing the problem; the value of the enum had to be used as an
array subscript, and the enum might well have been unsigned for all we
knew - arbitrary third party code is not to be trusted.

Could you explain this in more detail, as it's still not clear to me
what postgres was doing, nor how you fixed it.

You're saying postgres took a user-defined enum? Through a template
parameter (I assume not - since your example is in C) or an int type?
(I assume so) - and you wanted to check whether some specific value
passed in was usable as an array index, even though other values
within the enum might not be?

I'm still rather confused about this,

- David

17

I'd say that it was the most natural way of
expressing the problem; the value of the enum had to be used as an
array subscript, and the enum might well have been unsigned for all we
knew - arbitrary third party code is not to be trusted.

Could you explain this in more detail, as it's still not clear to me
what postgres was doing, nor how you fixed it.

There was an array that had values that are globally associated with
each enum constant. Which value in that array corresponds to which
enum constant is dictated by the enum constants underlying value - we
explicitly give the first literal in the enum the value 0, just to be
clear about our intent. Third party client code calls a function to
find out the value in the array that corresponds to some given enum
value. However, these clients cannot be trusted to pass a valid enum
literal, and not pass a value past the end of an array, or a negative
integer. Also, we cannot assume that the compiler has any particular
preference as to when it represents enums as signed or unsigned.

You're saying postgres took a user-defined enum?

No. It was not user-defined. It was simply an enum defined in a
header, included as part of the TU that this function definition is
in, as well as being included in a great deal of client code.

18

Yeah, then I'm with John on this - switching to the canonical
representation (that he deliberately provided support for) of
"first_enum <= value && value <= last_enum" is the right way to
express that.

If there's some other case where testing "enum_value > 0" could be
justified, then perhaps we could talk about that - but without a use
case it doesn't seem worth addressing on such theoretical grounds.

- David

19

Notably, Clang fairly arbitrarily doesn't show the warning when the
enum constant is used in place of a 0 rvalue. Why?

20

As per John's reply earlier/yesterday:

" When I
was implementing it, I was persuaded that we ought to have a
special case for comparing against an enumerator which happens
to be zero, because that was indicative of a particular common
and harmless idiom (specifically, things like if (v < v_first || v > v_last))."