This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Enum conversion warning when one signed and other unsigned.

Authored by red1bluelost on Apr 3 2022, 7:45 PM.



Ensures an -Wenum-conversion warning happens when one of the enums is
signed and the other is unsigned. Also adds a test file to verify these

This warning would not happen since the -Wsign-conversion would make a
diagnostic then return, never allowing the -Wenum-conversion checks.

For example:

enum PE { P = -1 };
enum NE { N };
enum NE conv(enum PE E) { return E; }

Before this would only create a diagnostic with -Wsign-conversion and never on
-Wenum-conversion. Now it will create a diagnostic for both -Wsign-conversion
and -Wenum-conversion.

I could change it to just warn on -Wenum-conversion as that was what I initially
did. Seeing PR35200 (or GitHub Issue 316268), I let both diagnostics check so that
the sign conversion could generate a warning.

This was tested with check-clang and all tests passed along with the new test

Diff Detail

Event Timeline

red1bluelost created this revision.Apr 3 2022, 7:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2022, 7:45 PM
red1bluelost requested review of this revision.Apr 3 2022, 7:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2022, 7:45 PM
aaron.ballman requested changes to this revision.Apr 19 2022, 10:40 AM

Thank you for working on this, and for your patience -- this review fell off my radar for a bit, sorry about that!

I think there's an issue here to be solved, but I'm not certain this is the correct way to solve it. Giving two or more diagnostics for the same underlying issue is generally something we try not to do, but happens as a result of your changes. However, this code should diagnose all four times, not just twice as it currently does, so there is a bug to fix:


I don't think this change is correct -- we *want* to silence the conversion warnings in this case. GCC behaves that way as well:


Testing tip: instead of using the preprocessor and duplicating so much, you can use a feature of -verify where you give it a prefix. e.g.,

// RUN: ... -verify=stuff ...
// RUN: ... -verify=other ...

void func(void) {
  thing();  // stuff-warning {{"this is a warning about stuff}} \
               other-error {{"this is an error, it's different than stuff}}

Using this sort of style makes the tests much easier to read.


Please add a newline to the end of the file.

This revision now requires changes to proceed.Apr 19 2022, 10:40 AM

Updates patch based on comments.

Silences the sign conversions on enum-to-enum so that only enum-to-enum will warn.
Simplifies the test case since now only one warning occurs.

red1bluelost marked 3 inline comments as done.Apr 20 2022, 4:14 PM
red1bluelost added inline comments.

Updated the code to silence the sign conversion and only warn the enum-conversion.


Thank! That is helpful to know!
I ended up not needing it here since the silencing of sign-conversion means only one warning will occur.

red1bluelost marked 2 inline comments as done.Apr 20 2022, 4:16 PM

Thanks for the updates! I think this is getting somewhat close, but I'd like to see some additional test cases to ensure we're not regressing behavior we care about (I think we may be losing warnings about sign conversion).

// Signed enums
enum SE1 { N1 = -1 };
enum SE2 { N2 = -2 };
// Unsigned unums
enum UE1 { P1 };
enum UE2 { P2 };

int f1(enum UE1 E) {
  return E; // warning about sign conversion

int f2(enum UE1 E) {
  return E; // warning about sign conversion

int f3(enum SE1 E) {
  return E; // shouldn't warn

int f4(enum SE1 E) {
  return E; // shouldn't warn

Also, before we land this, you should add a release note to clang/docs/ReleaseNotes.rst about the bugfix.

Adds regression tests for enum to int sign conversion warnings and notes the
changes in the release notes.

aaron.ballman added inline comments.May 3 2022, 12:08 PM

Minor tweak


Unrelated formatting change?


The second variants are unused.


Shouldn't these not warn as well -- the underlying enum type is unsigned?

red1bluelost marked 4 inline comments as done.

Addresses comments to improve wording and remove unnecessary code.




Changed back.


Good catch, removed.


This is the case with 'win32' for the ABI. It has enum be signed as default. I changed the name of the verify to win32 to that it is easier to see that is the case.

aaron.ballman accepted this revision.May 6 2022, 5:44 AM

LGTM aside from some minor corrections. Btw, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?


Almost, there's still whitespace changes. :-)


Then use E1 in place of UE1 -- it was the name UE1 plus the comment that kept tripping me up.


Ah, thanks, based on that, I have some other naming suggestions.

This revision is now accepted and ready to land.May 6 2022, 5:44 AM
red1bluelost marked 2 inline comments as done.

Addresses the last couple comments.

red1bluelost marked an inline comment as done.May 6 2022, 7:02 AM

If that's all good, could you commit on my behalf. Here is name and email: Micah Weston (

Thanks for the great reviews! It was really helpful with learning the frontend stuff.

aaron.ballman closed this revision.May 9 2022, 7:17 AM

Thank you for the fix! I've landed on your behalf in 882915df61e33f3a2b7f58e52f572717e1c11499.