Skip to content

Conversation

@erichkeane
Copy link
Collaborator

As brought up on the review for #147514, the original patch doesn't correctly parenthesize the expression in the assert. This fixes it.

As brought up on the review for llvm#147514, the original patch doesn't
correctly parenthesize the expression in the assert.  This fixes it.
@erichkeane erichkeane requested a review from cor3ntin September 15, 2025 13:26
@erichkeane erichkeane enabled auto-merge (squash) September 15, 2025 13:26
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Sep 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 15, 2025

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

As brought up on the review for #147514, the original patch doesn't correctly parenthesize the expression in the assert. This fixes it.


Full diff: https://github.com/llvm/llvm-project/pull/158635.diff

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGExprScalar.cpp (+3-3)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index ce483c5cc4e45..4fa25c5d66669 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2142,9 +2142,9 @@ Value *ScalarExprEmitter::VisitInitListExpr(InitListExpr *E) {
   bool Ignore = TestAndClearIgnoreResultAssign();
   (void)Ignore;
   unsigned NumInitElements = E->getNumInits();
-  assert(Ignore == false ||
-         (NumInitElements == 0 && E->getType()->isVoidType()) &&
-             "init list ignored");
+  assert((Ignore == false ||
+          (NumInitElements == 0 && E->getType()->isVoidType())) &&
+         "init list ignored");
 
   // HLSL initialization lists in the AST are an expansion which can contain
   // side-effecting expressions wrapped in opaque value expressions. To properly

@llvmbot
Copy link
Member

llvmbot commented Sep 15, 2025

@llvm/pr-subscribers-clang-codegen

Author: Erich Keane (erichkeane)

Changes

As brought up on the review for #147514, the original patch doesn't correctly parenthesize the expression in the assert. This fixes it.


Full diff: https://github.com/llvm/llvm-project/pull/158635.diff

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGExprScalar.cpp (+3-3)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index ce483c5cc4e45..4fa25c5d66669 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2142,9 +2142,9 @@ Value *ScalarExprEmitter::VisitInitListExpr(InitListExpr *E) {
   bool Ignore = TestAndClearIgnoreResultAssign();
   (void)Ignore;
   unsigned NumInitElements = E->getNumInits();
-  assert(Ignore == false ||
-         (NumInitElements == 0 && E->getType()->isVoidType()) &&
-             "init list ignored");
+  assert((Ignore == false ||
+          (NumInitElements == 0 && E->getType()->isVoidType())) &&
+         "init list ignored");
 
   // HLSL initialization lists in the AST are an expansion which can contain
   // side-effecting expressions wrapped in opaque value expressions. To properly

@erichkeane erichkeane changed the title [NFC] Fix commas on assertion from 147514 [NFC] Fix Parens on assertion from 147514 Sep 15, 2025
@brunodf-snps
Copy link
Contributor

Thanks for picking this up!

Comment on lines +2145 to +2147
assert((Ignore == false ||
(NumInitElements == 0 && E->getType()->isVoidType())) &&
"init list ignored");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following alternative is perhaps more clear, and allows to remove the (void)Ignore line from just above.

Suggested change
assert((Ignore == false ||
(NumInitElements == 0 && E->getType()->isVoidType())) &&
"init list ignored");
if (Ignore)
assert(NumInitElements == 0 && E->getType()->isVoidType()
&& "only ignore irrelevant init list");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That version ends up having a weird dead branch in non-asserts builds, and is problematic as assert is a macro. I don't see the losing the cast as worth the additional risk/breaking up the purpose of the assert here, so I'm going to leave it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, good to me. The extra parentheses solve the GCC warning.

@erichkeane erichkeane merged commit 92dcbf4 into llvm:main Sep 15, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants