This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr
ClosedPublic

Authored by shafik on Nov 29 2022, 1:34 PM.

Details

Summary

Currently Sema::BuildCXXTypeConstructExpr asserts that list initialization must mean we have an InitListExpr as well. We have several cases of valid code the result in CXXTemporaryObjectExpr in the AST instead for list initialization. Commit 1ae689c seems to indicate that this is not unexpected, although may be a design issue.

This fixes:

https://github.com/llvm/llvm-project/issues/58302
https://github.com/llvm/llvm-project/issues/58753
https://github.com/llvm/llvm-project/issues/59100

Diff Detail

Event Timeline

shafik created this revision.Nov 29 2022, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 1:34 PM
shafik requested review of this revision.Nov 29 2022, 1:34 PM

The windows failure happened on the build before as well so not related to this change: https://buildkite.com/llvm-project/premerge-checks/builds/123890

cor3ntin added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
1464

Exprs.size() == 1 should still be true, right? Is it worth keeping? Maybe?
Similarly, is ListInitialization the right name here? Maybe ParsedAsListInitialization or something? Or a Fixme?

shafik added inline comments.Nov 29 2022, 6:34 PM
clang/lib/Sema/SemaExprCXX.cpp
1464

I believe ListInitialization is still correct as I mentioned in this bug report: https://github.com/llvm/llvm-project/issues/58302 it is still list initialization.

You have a point it may be worth it to just remove && isa<InitListExpr>(Exprs[0])

Exprs.size() == 1 is still valid in every example we've seen, so yes, you might want to keep it (and update the assertion message)

shafik updated this revision to Diff 479161.Nov 30 2022, 9:35 PM

Bring back assert but w/ the check for InitListExpr removed.

shafik retitled this revision from [Clang] Remove invalid assert from Sema::BuildCXXTypeConstructExpr to [Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr.Nov 30 2022, 9:36 PM
davide accepted this revision.Nov 30 2022, 9:37 PM

LGTM with the comment reworded. Thanks.

clang/lib/Sema/SemaExprCXX.cpp
1463

I'd rephrase this as "List initialization must have exactly one expression"

This revision is now accepted and ready to land.Nov 30 2022, 9:37 PM
aaron.ballman accepted this revision.Dec 1 2022, 6:16 AM

LGTM with the rewording suggestion applied. Please also add a release note for the fix, thank you!

shafik updated this revision to Diff 479332.Dec 1 2022, 9:23 AM
  • Update assert wording
  • Add release note
davide added a comment.Dec 1 2022, 9:24 AM

LGTM, again.

This revision was landed with ongoing or failed builds.Dec 1 2022, 9:40 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 9:40 AM