-
Notifications
You must be signed in to change notification settings - Fork 15k
[clang] Accept empty enum in MSVC compatible C #159981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: yicuixi <[email protected]>
|
@llvm/pr-subscribers-clang Author: None (yicuixi) ChangesFixes #114402. Full diff: https://github.com/llvm/llvm-project/pull/159981.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 46d56bb3f07f5..c4b5e15b2b4f9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -366,6 +366,7 @@ Bug Fixes in This Version
- Fixed an assertion when an improper use of the ``malloc`` attribute targeting
a function without arguments caused us to try to access a non-existent argument.
(#GH159080)
+- Accept empty enum in MSVC-compatible C. (#GH114402)
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 22c01c4e371f3..17e29b3efc348 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5360,7 +5360,8 @@ void Parser::ParseEnumBody(SourceLocation StartLoc, Decl *EnumDecl,
T.consumeOpen();
// C does not allow an empty enumerator-list, C++ does [dcl.enum].
- if (Tok.is(tok::r_brace) && !getLangOpts().CPlusPlus)
+ if (Tok.is(tok::r_brace) && !getLangOpts().CPlusPlus &&
+ !getLangOpts().MSVCCompat && !getLangOpts().MicrosoftExt)
Diag(Tok, diag::err_empty_enum);
SmallVector<Decl *, 32> EnumConstantDecls;
diff --git a/clang/test/Parser/ms-empty-enum.c b/clang/test/Parser/ms-empty-enum.c
new file mode 100644
index 0000000000000..37e34d7c154bf
--- /dev/null
+++ b/clang/test/Parser/ms-empty-enum.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-compatibility
+// expected-no-diagnostics
+
+typedef enum tag1 { } A;
+typedef enum tag2 { } B;
+typedef enum : unsigned { } C;
+
|
Signed-off-by: yicuixi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IF we're accepting this as an extension, we need to emit a diagnostic to that effect. See the Ext-Warn already with an underlying type (which we already supported), we need to do something similar for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain the problem in the summary instead of just pointing to an issue. The summary should explain basics required to understand the PR w/o having to jump to the issue right away for context.
So, What are you changing, why are you changing it and how.
Done |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Ping @erichkeane |
|
|
||
| typedef enum tag1 { } A; // expected-warning {{empty enumeration types are a Microsoft extension}} | ||
| typedef enum tag2 { } B; // expected-warning {{empty enumeration types are a Microsoft extension}} | ||
| typedef enum : unsigned { } C; // expected-warning {{enumeration types with a fixed underlying type are a Microsoft extension}}\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't warn 2x, the 'with a fixed underlying type' version is sufficient.
Also, isn't this a C23 extension, not a microsoft extension (or at least, as well)? See: ext_c23_enum_fixed_underlying_type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
Also, isn't this a C23 extension, not a microsoft extension (or at least, as well)? See: ext_c23_enum_fixed_underlying_type.
In microsoft-extension mode, it's ext_ms_c_enum_fixed_underlying_type but not ext_ms_c_enum_fixed_underlying_type.
This shouldn't warn 2x, the 'with a fixed underlying type' version is sufficient.
Should we suppress the 2nd empty enumeration types are a Microsoft extension warning? Seems they are 2 different issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused here as to the order of warnings/what we should do. I suspect @AaronBallman and I need to spend some time figuring out/discussing what we're looking for.
I'm not sure about the suppressing the 2nd warning, there is perhaps a 'we should do both' kinda thing since one might be disabled, but it also looks silly.
Ping this in another ~week and a half, and Aaron and I can discuss it (he should be back by then!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think emitting two diagnostics is fine (one for the underlying type and one for the empty enum) because they are separate extensions. But I think it's a bit odd that we claim the underlying type is a Microsoft extension rather than a C23 extension -- that seems to be a preexisting issue which could be handled in a separate PR: https://godbolt.org/z/sMhGoE3qq
address comments Co-authored-by: Erich Keane <[email protected]>
|
@erichkeane friendly ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronBallman is back now! He will hopefully be getting to this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
I'd like to see extra test coverage to ensure that an empty enum has the same ABI properties in Clang as it does in MSVC. e.g., the size of the enumeration is still the same as the size of an int, an object of that type is still passed as an int (no masking or other oddities), etc. So I think perhaps we should add a CodeGen test just to smoke test the behavior.
We should probably also mention the extension somewhere in clang/docs/LanguageExtensions.rst. We currently have a section for Enumerations with a fixed underlying type, so we could add one nearby for Enumerations with no enumerators or something.
|
|
||
| typedef enum tag1 { } A; // expected-warning {{empty enumeration types are a Microsoft extension}} | ||
| typedef enum tag2 { } B; // expected-warning {{empty enumeration types are a Microsoft extension}} | ||
| typedef enum : unsigned { } C; // expected-warning {{enumeration types with a fixed underlying type are a Microsoft extension}}\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think emitting two diagnostics is fine (one for the underlying type and one for the empty enum) because they are separate extensions. But I think it's a bit odd that we claim the underlying type is a Microsoft extension rather than a C23 extension -- that seems to be a preexisting issue which could be handled in a separate PR: https://godbolt.org/z/sMhGoE3qq
Signed-off-by: yicuixi <[email protected]>
Thanks for the review and sorry for the very late update. I've added a new CodeGen test. |
SGTM, I'd prefer to fix that in a separate patch. |
Signed-off-by: yicuixi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM aside from some nits with documentation
| Enumerations with no enumerators | ||
| ----------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Enumerations with no enumerators | |
| ----------------------------------------- | |
| Enumerations with no enumerators | |
| -------------------------------- |
| Enumerations with no enumerators | ||
| ----------------------------------------- | ||
| Clang provides support for Microsoft extensions to support empty enums. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Clang provides support for Microsoft extensions to support empty enums. | |
| Clang provides support for Microsoft extensions to support enumerations with no enumerators. |
Fixes #114402.
This patch accept empty enum in C as a microsoft extension and introduce an new warning
-Wmicrosoft-empty-enum.