Regression (r316268): erroneous -Wsign-compare for typeof(enum T) and typeof(enumValue)

[hyp]

Bugzilla Link	36008
Resolution	FIXED
Resolved on	Feb 08, 2018 05:19
Version	trunk
OS	All
Blocks	#35152
CC	@AaronBallman,@hyp,@efriedma-quic,@zmodem,@LebedevRI,@zygoloid

Extended Description

In LLVM 6 Clang reports -Wsign-compare:

comparison of integers of different signs: 'typeof (lhs)' (aka 'enum EnumTest') and 'typeof (kValue)' (aka 'int')

For the following sample:

enum EnumTest {
kValue = 0,
};

void foo(enum EnumTest lhs) {
typeof(lhs) x = lhs;
typeof(kValue) y = kValue;
x == y;
}

This is a regression after r316268.

[LebedevRI]

Hmm, interesting.

It happens with -triple=x86_64-pc-linux-gnu, where enum's underlying datatype
is unsigned by default, but obviously does not happen with
-triple=x86_64-pc-win32 where enum's underlying datatype is signed.

https://godbolt.org/g/5E7Vhj
typeof(kValue) is int.
So to me at least it isn't illogical that the warning is present.

If you change kValue to -10, the enum becomes signed, and the warning disappears.

[hyp]

It's not that great to report a warning only for the specific target.
What's worse is that our users have to silence this warning by using explicit cast in code where the enum constant is used directly (i.e. in a macro), e.g. code like this:

#define is(x, y) ({ typeof(x) a = x; typeof(y)__ b = y; x == y; })

enum EnumTest returns();

void test() {
is(returns(), kValue);
}

has to be fixed with a cast on kValue:

void test() {
is(returns(), (enum EnumTest)kValue);
}

[LebedevRI]

It's not that great to report a warning only for the specific target.
Yeah, see r322901 and the stuff referenced there.

What's worse is that our users have to silence this warning by using
explicit cast in code where the enum constant is used directly (i.e. in a
macro), e.g. code like this:

#define is(x, y) ({ typeof(x) a = x; typeof(y)__ b = y; x == y; })

enum EnumTest returns();

void test() {
is(returns(), kValue);
}

has to be fixed with a cast on kValue:

void test() {
is(returns(), (enum EnumTest)kValue);
}

Hmm, without context, this seems rather over-engineered.
Just for the reference, this does not warn, and should not warn:

enum EnumTest {
kValue = 0,
};

enum EnumTest returns();

void test() {
returns() == kValue;
}

I.e. comparison of the enum value with the enumerator is not diagnosed, intentionally.

[hyp]

Created a patch https://reviews.llvm.org/D42561

[hyp]

r324514.

[LebedevRI]

Thank you!

[zmodem]

Let's keep it open until we decide whether to merge to 6.0.

Richard, Aaron, what do you think?

[AaronBallman]

Let's keep it open until we decide whether to merge to 6.0.

Richard, Aaron, what do you think?

I am fine merging this -- it's small and uncontroversial, IMO.

[zmodem]

Let's keep it open until we decide whether to merge to 6.0.

Richard, Aaron, what do you think?

I am fine merging this -- it's small and uncontroversial, IMO.

Okay, r324602.