This is an archive of the discontinued LLVM Phabricator instance.

Suppress Wsign-conversion for enums with matching underlying type
ClosedPublic

Authored by erichkeane on Sep 21 2017, 10:49 AM.

Details

Summary

As reported here: https://bugs.llvm.org/show_bug.cgi?id=34692

A non-defined enum with a backing type was always defaulting to
being treated as a signed type. IN the case where it IS defined,
the signed-ness of the actual items is used.

This patch uses the underlying type's signed-ness in the non-defined
case to test signed-comparision.

Diff Detail

Event Timeline

erichkeane created this revision.Sep 21 2017, 10:49 AM
efriedma added inline comments.
lib/Sema/SemaChecking.cpp
8179

Maybe add a comment noting what can trigger this case?

test/SemaCXX/sign-conversion.cpp
21 ↗(On Diff #116220)

There's an existing file test/SemaCXX/warn-sign-conversion.cpp for C++ sign-conversion tests.

erichkeane marked 2 inline comments as done.

Updated based on Eli's feedback.

efriedma added inline comments.Sep 21 2017, 12:27 PM
lib/Sema/SemaChecking.cpp
8176

Explicitly referencing sign-conversion warnings here isn't really helpful. Maybe something more like "Incomplete enums without definitions can have an explicitly specified underlying type. Use that type here to compute the range."

test/SemaCXX/warn-sign-conversion-cpp11.cpp
11

The signedness of "char" can vary based on the host; probably simplest to write out "signed char".

erichkeane marked an inline comment as done.
erichkeane added inline comments.
lib/Sema/SemaChecking.cpp
8176

Ok, got it. I DO note that (IIRC) enums with an underlying type are not considered 'incomplete', so I modified your text slightly. Thanks for the help.

test/SemaCXX/sign-conversion.cpp
21 ↗(On Diff #116220)

Thanks for pointing that out! Unfortunately, that file is very dependent on it being C++98, and this test requires c++11 or greater. I DID rename the test file to better match that one however.

test/SemaCXX/warn-sign-conversion-cpp11.cpp
11

Right, good catch.

This revision is now accepted and ready to land.Sep 21 2017, 12:34 PM
This revision was automatically updated to reflect the committed changes.