[clang] Remove hasValue() check in RecordExprEvaluator::VisitCXXConstructExpr()
#154610
Conversation
|
@llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) ChangesLet's see if this fixes #154567 without regressions. Full diff: https://github.com/llvm/llvm-project/pull/154610.diff 1 Files Affected:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 40c56501b0c14..c51f132614667 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -11046,8 +11046,8 @@ bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E,
bool ZeroInit = E->requiresZeroInitialization();
if (CheckTrivialDefaultConstructor(Info, E->getExprLoc(), FD, ZeroInit)) {
// If we've already performed zero-initialization, we're already done.
- if (Result.hasValue())
- return true;
+ if (Result.hasValue()) {
+ }
if (ZeroInit)
return ZeroInitialization(E, T);
|
|
Thanks for having a look so quickly! |
|
Does a release note make sense if the patch gets backported to the release branch anyway? |
Nope! I didn't realize this was heading for a backport |
|
@philnik777 Merge this whenever you need it, otherwise I'd leave it open for one of the other reviewers to look at. |
|
@tbaederr Thanks! I'll merge it once the fix on our side is ready. |
|
/cherry-pick 8b3d4bd |
|
Failed to cherry-pick: 8b3d4bd https://github.com/llvm/llvm-project/actions/runs/17173100689 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
| @@ -11045,10 +11045,6 @@ bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E, | |||
|
|
|||
| bool ZeroInit = E->requiresZeroInitialization(); | |||
| if (CheckTrivialDefaultConstructor(Info, E->getExprLoc(), FD, ZeroInit)) { | |||
| // If we've already performed zero-initialization, we're already done. | |||
| if (Result.hasValue()) | |||
There was a problem hiding this comment.
Was this code do nothing then? It looks like this check came in with e637cbe
There was a problem hiding this comment.
When it's working correctly, it allows skipping work zero-initializing something that's already zero-initialized, I think. But that assumes we maintain an invariant about exactly when values are absent, vs. uninitialized. We don't maintain that invariant for anonymous structs. (We probably could, but it's a little tricky to implement correctly for nested anonymous structs.)
There was a problem hiding this comment.
Makes sense, maybe worth a FIXME, probably not a huge win though.
…tructExpr()` (llvm#154610) When initializing an anonymous struct via an `IndirectFieldDecl`, we create an `APValue` for the struct, but we leave the fields uninitialized. This would later cause the `CXXConstructExpr` that initializes the anonymous struct member to not do anything since its `APValue` already had a value (but the member didn't). Just remove the check for an `APValue` that already has a value from `RecordExprEvaluator::VisitCXXConstructExpr()`. Fixes llvm#154567
…tructExpr()` (llvm#154610) When initializing an anonymous struct via an `IndirectFieldDecl`, we create an `APValue` for the struct, but we leave the fields uninitialized. This would later cause the `CXXConstructExpr` that initializes the anonymous struct member to not do anything since its `APValue` already had a value (but the member didn't). Just remove the check for an `APValue` that already has a value from `RecordExprEvaluator::VisitCXXConstructExpr()`. Fixes llvm#154567
For completeness, this was done in 3263ad7 |
When initializing an anonymous struct via an
IndirectFieldDecl, we create anAPValuefor the struct, but we leave the fields uninitialized. This would later cause theCXXConstructExprthat initializes the anonymous struct member to not do anything since itsAPValuealready had a value (but the member didn't). Just remove the check for anAPValuethat already has a value fromRecordExprEvaluator::VisitCXXConstructExpr().Fixes #154567