[Clang][Backport] Demote mixed enumeration arithmetic error to a warning (#131811) #139396
Conversation
|
@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/139396.diff 4 Files Affected:
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/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index e440f60526bb3..00f7f87838cf1 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -14688,7 +14688,21 @@ bool Sema::BuiltinVectorToScalarMath(CallExpr *TheCall) {
return false;
}
-bool Sema::BuiltinVectorMath(CallExpr *TheCall, QualType &Res, bool FPOnly) {
+static bool checkBuiltinVectorMathMixedEnums(Sema &S, Expr *LHS, Expr *RHS,
+ SourceLocation Loc) {
+ QualType L = LHS->getEnumCoercedType(S.Context),
+ R = RHS->getEnumCoercedType(S.Context);
+ if (L->isUnscopedEnumerationType() && R->isUnscopedEnumerationType() &&
+ !S.Context.hasSameUnqualifiedType(L, R)) {
+ return S.Diag(Loc, diag::err_conv_mixed_enum_types)
+ << LHS->getSourceRange() << RHS->getSourceRange()
+ << /*Arithmetic Between*/ 0 << L << R;
+ }
+ return false;
+}
+
+std::optional<QualType> Sema::BuiltinVectorMath(CallExpr *TheCall,
+ bool FPOnly) {
if (checkArgCount(TheCall, 2))
return true;
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')}}
}
|
…131811) In C++, defaulted to an error. C++ removed these features but the removal negatively impacts users. Fixes llvm#92340
77918bb to
e43f7db
Compare
| 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; |
There was a problem hiding this comment.
Unfortunately our ABI checker job doesn't really work, but I think this is an ABI break, because it's adding a new enum value.
There was a problem hiding this comment.
I don't disagree (it does add a new enum value), but I am starting to think this ABI requirement is onerous enough to be worth rethinking the scope of it. The point to the ABI requirement is that you should be able to use Clang as a library interchangeably between the dot releases. The platform calling convention ABI does not change when an enumeration gains a new enumerator, nor does name mangling behavior change. In fact, adding an enumerator can only increase the number of valid integer values that can be represented by the enumeration, which means the addition won't introduce new UB but could remove UB by widening the range of valid values. So I think this kind of change isn't introducing a kind of ABI break we need to avoid.
Or am I missing something?
There was a problem hiding this comment.
The issue is that TableGen sorts the enums alphabetically, so adding a new value will change the name->integer mapping for approximately half of the enum. Maybe ABI break is the wrong term here, but it means than an app built against 20.1.4, for example will stop working if 20.1.4 libraries are replaced by 20.1.5 libraries, which is something we want to avoid even if it's not technically an ABI break.
There was a problem hiding this comment.
So presumably, if we renamed it to z_error_xxx it would be fine?
There was a problem hiding this comment.
I think so, yes. Someone could verify by looking at the generated .inc file.
In C++, defaulted to an error.
C++ removed these features, but the removal negatively impacts users.
Fixes #92340
Backports #131811