-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[Clang][Backport] Demote mixed enumeration arithmetic error to a warning #131853
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
Conversation
…ing (llvm#131811) In C++, defaulted to an error. C++ removed these features but the removal negatively impacts users. Fixes llvm#92340
|
@llvm/pr-subscribers-clang Author: cor3ntin (cor3ntin) ChangesIn C++, defaulted to an error. C++ removed these features but the removal negatively impacts users. Fixes #92340 Full diff: https://github.com/llvm/llvm-project/pull/131853.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 02292c10e6964..04e5d89dfcde1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -299,6 +299,9 @@ C++ Language Changes
- The builtin type alias ``__builtin_common_type`` has been added to improve the
performance of ``std::common_type``.
+- In C++26 mode, arithmetic conversion errors between mixed enumeration types can
+ be disabled with ``-Wno-enum-enum-conversion`` (#GH92340).
+
C++2c Feature Support
^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index ec2a140e04d5b..6c93a46d8f36c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7567,9 +7567,13 @@ def warn_arith_conv_mixed_enum_types_cxx20 : Warning<
"%sub{select_arith_conv_kind}0 "
"different enumeration types%diff{ ($ and $)|}1,2 is deprecated">,
InGroup<DeprecatedEnumEnumConversion>;
-def err_conv_mixed_enum_types_cxx26 : Error<
+
+def err_conv_mixed_enum_types: Error <
"invalid %sub{select_arith_conv_kind}0 "
"different enumeration types%diff{ ($ and $)|}1,2">;
+def warn_conv_mixed_enum_types_cxx26 : Warning <
+ err_conv_mixed_enum_types.Summary>,
+ InGroup<EnumEnumConversion>, DefaultError;
def warn_arith_conv_mixed_anon_enum_types : Warning<
warn_arith_conv_mixed_enum_types.Summary>,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index e253e3a17328f..eae7f1c3aa781 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -1518,8 +1518,8 @@ static void checkEnumArithmeticConversions(Sema &S, Expr *LHS, Expr *RHS,
unsigned DiagID;
// In C++ 26, usual arithmetic conversions between 2 different enum types
// are ill-formed.
- if (S.getLangOpts().CPlusPlus26)
- DiagID = diag::err_conv_mixed_enum_types_cxx26;
+ if (getLangOpts().CPlusPlus26)
+ DiagID = diag::warn_conv_mixed_enum_types_cxx26;
else if (!L->castAs<EnumType>()->getDecl()->hasNameForLinkage() ||
!R->castAs<EnumType>()->getDecl()->hasNameForLinkage()) {
// If either enumeration type is unnamed, it's less likely that the
diff --git a/clang/test/SemaCXX/cxx2c-enum-compare.cpp b/clang/test/SemaCXX/cxx2c-enum-compare.cpp
index f47278a60725e..96fbd368b1696 100644
--- a/clang/test/SemaCXX/cxx2c-enum-compare.cpp
+++ b/clang/test/SemaCXX/cxx2c-enum-compare.cpp
@@ -1,9 +1,10 @@
-// RUN: %clang_cc1 %s -std=c++2c -fsyntax-only -verify -triple %itanium_abi_triple
+// RUN: %clang_cc1 %s -std=c++2c -fsyntax-only -verify=both,expected
+// RUN: %clang_cc1 %s -std=c++2c -fsyntax-only -verify=both -Wno-enum-enum-conversion
enum E1 { e };
enum E2 { f };
void test() {
- int b = e <= 3.7; // expected-error {{invalid comparison of enumeration type 'E1' with floating-point type 'double'}}
+ int b = e <= 3.7; // both-error {{invalid comparison of enumeration type 'E1' with floating-point type 'double'}}
int k = f - e; // expected-error {{invalid arithmetic between different enumeration types ('E2' and 'E1')}}
int x = 1 ? e : f; // expected-error {{invalid conditional expression between different enumeration types ('E1' and 'E2')}}
}
|
AaronBallman
left a comment
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.
LGTM
|
It looks like there are test failures introduced by this patch. |
2e8de29 to
00c10b7
Compare
|
|
||
| def err_conv_mixed_enum_types: Error < | ||
| "invalid %sub{select_arith_conv_kind}0 " | ||
| "different enumeration types%diff{ ($ and $)|}1,2">; | ||
| def warn_conv_mixed_enum_types_cxx26 : Warning < | ||
| err_conv_mixed_enum_types.Summary>, | ||
| InGroup<EnumEnumConversion>, DefaultError; |
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 this may be an ABI break if it changes enum values in the generated header.
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.
How do you usually deal with that?
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.
We usually just don't take these kinds of packages, but there may be a way to name the new option so that it ends up at the end of the enum list.
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 don't know of a reliable way to ensure the diagnostic is at the end of the list. I think putting it at the end of DiagnosticSemaKinds.td will suffice (we allocate diagnostic IDs in blocks based on the .td file they're in). However, I'm not certain that's a guarantee.
Perhaps we don't backport this?
shafik
left a comment
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.
LGTM
In C++, defaulted to an error.
C++ removed these features but the removal negatively impacts users.
Fixes #92340