[C] Handle comma operator for implicit int->enum conversions #138752
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In C++, the type of an enumerator is the type of the enumeration,
whereas in C, the type of the enumerator is 'int'. The type of a comma
operator is the type of the right-hand operand, which means you can get
an implicit conversion with this code in C but not in C++:
enum E { Zero };
enum E foo() {
return ((void)0, Zero);
}
We were previously incorrectly diagnosing this code as being
incompatible with C++ because the type of the paren expression would be
'int' there, whereas in C++ the type is 'E'.
So now we handle the comma operator with special logic when analyzing
implicit conversions in C. When analyzing the left-hand operand of a
comma operator, we do not need to check for that operand causing an
implicit conversion for the entire comma expression. So we only check
for that case with the right-hand operand.
This addresses a concern brought up post-commit:
llvm#137658 (comment)
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesIn C++, the type of an enumerator is the type of the enumeration, whereas in C, the type of the enumerator is 'int'. The type of a comma operator is the type of the right-hand operand, which means you can get an implicit conversion with this code in C but not in C++: We were previously incorrectly diagnosing this code as being incompatible with C++ because the type of the paren expression would be 'int' there, whereas in C++ the type is 'E'. So now we handle the comma operator with special logic when analyzing implicit conversions in C. When analyzing the left-hand operand of a comma operator, we do not need to check for that operand causing an implicit conversion for the entire comma expression. So we only check for that case with the right-hand operand. This addresses a concern brought up post-commit: Full diff: https://github.com/llvm/llvm-project/pull/138752.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 7f45533713bae..5f7769f9758a6 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11647,6 +11647,29 @@ static void DiagnoseFloatingImpCast(Sema &S, Expr *E, QualType T,
}
}
+static void CheckCommaOperand(Sema &S, Expr *E, QualType T, SourceLocation CC,
+ bool Check) {
+ E = E->IgnoreParenImpCasts();
+ AnalyzeImplicitConversions(S, E, CC);
+
+ if (Check && E->getType() != T)
+ S.CheckImplicitConversion(E, T, CC);
+}
+
+/// Analyze the given comma operator. The basic idea behind the analysis is to
+/// analyze the left and right operands slightly differently. The left operand
+/// needs to check whether the operand itself has an implicit conversion, but
+/// not whether the left operand induces an implicit conversion for the entire
+/// comma expression itself. This is similar to how CheckConditionalOperand
+/// behaves; it's as-if the correct operand were directly used for the implicit
+/// conversion check.
+static void AnalyzeCommaOperator(Sema &S, BinaryOperator *E, QualType T) {
+ assert(E->isCommaOp() && "Must be a comma operator");
+
+ CheckCommaOperand(S, E->getLHS(), T, E->getOperatorLoc(), false);
+ CheckCommaOperand(S, E->getRHS(), T, E->getOperatorLoc(), true);
+}
+
/// Analyze the given compound assignment for the possible losing of
/// floating-point precision.
static void AnalyzeCompoundAssignment(Sema &S, BinaryOperator *E) {
@@ -12464,7 +12487,7 @@ static void AnalyzeImplicitConversions(
<< OrigE->getSourceRange() << T->isBooleanType()
<< FixItHint::CreateReplacement(UO->getBeginLoc(), "!");
- if (const auto *BO = dyn_cast<BinaryOperator>(SourceExpr))
+ if (auto *BO = dyn_cast<BinaryOperator>(SourceExpr))
if ((BO->getOpcode() == BO_And || BO->getOpcode() == BO_Or) &&
BO->getLHS()->isKnownToHaveBooleanValue() &&
BO->getRHS()->isKnownToHaveBooleanValue() &&
@@ -12490,6 +12513,9 @@ static void AnalyzeImplicitConversions(
(BO->getOpcode() == BO_And ? "&&" : "||"));
S.Diag(BO->getBeginLoc(), diag::note_cast_operand_to_int);
}
+ } else if (BO->isCommaOp() && !S.getLangOpts().CPlusPlus) {
+ AnalyzeCommaOperator(S, BO, T);
+ return;
}
// For conditional operators, we analyze the arguments as if they
diff --git a/clang/test/Sema/implicit-cast.c b/clang/test/Sema/implicit-cast.c
index 088b1958d9b85..4700b7d37a855 100644
--- a/clang/test/Sema/implicit-cast.c
+++ b/clang/test/Sema/implicit-cast.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
static char *test1(int cf) {
return cf ? "abc" : 0;
@@ -6,3 +6,8 @@ static char *test1(int cf) {
static char *test2(int cf) {
return cf ? 0 : "abc";
}
+
+int baz(void) {
+ int f;
+ return ((void)0, f = 1.4f); // expected-warning {{implicit conversion from 'float' to 'int' changes value from 1.4 to 1}}
+}
diff --git a/clang/test/Sema/implicit-int-enum-conversion.c b/clang/test/Sema/implicit-int-enum-conversion.c
index 13afb5d297aba..36717f36dd083 100644
--- a/clang/test/Sema/implicit-int-enum-conversion.c
+++ b/clang/test/Sema/implicit-int-enum-conversion.c
@@ -50,3 +50,25 @@ enum E1 quux(void) {
return E2_Zero; // expected-warning {{implicit conversion from enumeration type 'enum E2' to different enumeration type 'enum E1'}} \
cxx-error {{cannot initialize return object of type 'enum E1' with an rvalue of type 'E2'}}
}
+
+enum E1 comma1(void) {
+ return ((void)0, E1_One);
+}
+
+enum E1 comma2(void) {
+ enum E1 x;
+ return
+ (x = 12, // expected-warning {{implicit conversion from 'int' to enumeration type 'enum E1' is invalid in C++}} \
+ cxx-error {{assigning to 'enum E1' from incompatible type 'int'}}
+ E1_One);
+}
+
+enum E1 comma3(void) {
+ enum E1 x;
+ return ((void)0, foo()); // Okay, no conversion in C++
+}
+
+enum E1 comma4(void) {
+ return ((void)1, 2); // expected-warning {{implicit conversion from 'int' to enumeration type 'enum E1' is invalid in C++}} \
+ cxx-error {{cannot initialize return object of type 'enum E1' with an rvalue of type 'int'}}
+}
|
erichkeane
reviewed
May 6, 2025
* Renamed a parameter to be more clear * Inlined a simple function body into its sole call site
|
I see that if I build clang with ASAN with this patch and run the testcase |
Thank you for letting me know! I'm trying to reproduce the issue on Windows with MSVC + ASAN and I'm not getting any failures. The stack trace looks valid, but also implies that |
|
I've run it as well this way: However, I'm not reproducing any problems with that. @mikaelholmen : Are there any other repro steps we have to do to enable this? |
|
@AaronBallman @erichkeane I made another try with a later build now and then I don't see the failure anymore so perhaps something has been fixed. Or it just went hiding but then I suppose it will popup at some point again. Sorry if I sent you off on a wild goose chase. :( |
No worries, I'm glad there's not an issue after all. :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In C++, the type of an enumerator is the type of the enumeration, whereas in C, the type of the enumerator is 'int'. The type of a comma operator is the type of the right-hand operand, which means you can get an implicit conversion with this code in C but not in C++:
We were previously incorrectly diagnosing this code as being incompatible with C++ because the type of the paren expression would be 'int' there, whereas in C++ the type is 'E'.
So now we handle the comma operator with special logic when analyzing implicit conversions in C. When analyzing the left-hand operand of a comma operator, we do not need to check for that operand causing an implicit conversion for the entire comma expression. So we only check for that case with the right-hand operand.
This addresses a concern brought up post-commit:
#137658 (comment)