[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6198-6199
+  
dyn_cast(DestType->getAs()->getDecl());
+  S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() &&
+  getFailureKind() == FK_ConstructorOverloadFailed &&
+  onlyHasDefaultedCtors(getFailedCandidateSet())) {

rsmith wrote:
> For when you look at re-landing this, we encountered a regression for this 
> case:
> 
> ```
> struct A {};
> struct B : A { Noncopyable nc; };
> ```
> 
> Here, given `B b`, a `B(b)` direct-initialization should be rejected because 
> it selects a deleted copy constructor of `B`, but it looks like Clang instead 
> performed aggregate initialization, as if by `B{(const A&)b, {}}`. This is 
> pretty bad -- copies that should be disallowed end up working but not 
> actually copying any members of the derived class!
> 
> The general problem is that Clang's overload resolution incorrectly handles 
> deleted functions, treating them as an overload resolution failure rather 
> than a success. In this particular case, what happens is constructor overload 
> resolution produces `OR_Deleted`, which gets mapped into 
> `FK_ConstructorOverloadFailed` despite the fact that overload resolution 
> succeeded and picked a viable function.
> 
> I guess we can split `FK_*OverloadFailed` into separate "failed" and 
> "succeeded but found a deleted function" cases?
> For when you look at re-landing this, we encountered a regression for this 
> case:
> 
> ```
> struct A {};
> struct B : A { Noncopyable nc; };
> ```
> 
> Here, given `B b`, a `B(b)` direct-initialization should be rejected because 
> it selects a deleted copy constructor of `B`, but it looks like Clang instead 
> performed aggregate initialization, as if by `B{(const A&)b, {}}`. This is 
> pretty bad -- copies that should be disallowed end up working but not 
> actually copying any members of the derived class!
> 
> The general problem is that Clang's overload resolution incorrectly handles 
> deleted functions, treating them as an overload resolution failure rather 
> than a success. In this particular case, what happens is constructor overload 
> resolution produces `OR_Deleted`, which gets mapped into 
> `FK_ConstructorOverloadFailed` despite the fact that overload resolution 
> succeeded and picked a viable function.
> 
> I guess we can split `FK_*OverloadFailed` into separate "failed" and 
> "succeeded but found a deleted function" cases?

This was the cause of https://github.com/llvm/llvm-project/issues/59675 as 
well. The reland at D141546 should handle this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6198-6199
+  
dyn_cast(DestType->getAs()->getDecl());
+  S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() &&
+  getFailureKind() == FK_ConstructorOverloadFailed &&
+  onlyHasDefaultedCtors(getFailedCandidateSet())) {

For when you look at re-landing this, we encountered a regression for this case:

```
struct A {};
struct B : A { Noncopyable nc; };
```

Here, given `B b`, a `B(b)` direct-initialization should be rejected because it 
selects a deleted copy constructor of `B`, but it looks like Clang instead 
performed aggregate initialization, as if by `B{(const A&)b, {}}`. This is 
pretty bad -- copies that should be disallowed end up working but not actually 
copying any members of the derived class!

The general problem is that Clang's overload resolution incorrectly handles 
deleted functions, treating them as an overload resolution failure rather than 
a success. In this particular case, what happens is constructor overload 
resolution produces `OR_Deleted`, which gets mapped into 
`FK_ConstructorOverloadFailed` despite the fact that overload resolution 
succeeded and picked a viable function.

I guess we can split `FK_*OverloadFailed` into separate "failed" and "succeeded 
but found a deleted function" cases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

FYI, this patch triggers a crash in chromium, see 
https://github.com/llvm/llvm-project/issues/59675 for details.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao marked 3 inline comments as done.
ayzhao added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:4760
+  : Expr(CXXParenListInitExprClass, T,
+ T->isLValueReferenceType()   ? VK_LValue
+ : T->isRValueReferenceType() ? VK_XValue

shafik wrote:
> It looks like we use this idiom in several places, it may be worth it to sink 
> this as a member function of `QualType`
note: this patch was already merged, so I created a new patch addressing the 
revisions at D140159

It turns out there's a method `getValueKindForType(...)` that does the exact 
same thing, so I used that instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:4760
+  : Expr(CXXParenListInitExprClass, T,
+ T->isLValueReferenceType()   ? VK_LValue
+ : T->isRValueReferenceType() ? VK_XValue

It looks like we use this idiom in several places, it may be worth it to sink 
this as a member function of `QualType`



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1609
+dyn_cast(E->getType()->castAs()->getDecl());
+for (FieldDecl *FD : RD->fields()) {
+  if (FD->isUnnamedBitfield())

This is second time I see you looking for the initialized field for a union 
while `InitListExpr` has `getInitializedFieldInUnion()`. Would it be too 
painful to also do that for `CXXParenListInitExpr ` to avoid the code 
duplication?



Comment at: clang/lib/Sema/SemaInit.cpp:5411
+  ResultType = S.Context.getConstantArrayType(
+  AT->getElementType(), llvm::APInt(32, ArrayLength), nullptr,
+  ArrayType::Normal, 0);

nit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG40c52159d3ee: [clang][C++20] P0960R3 and P1975R0: Allow 
initializing aggregates from a… (authored by ayzhao).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 482862.
ayzhao marked an inline comment as done.
ayzhao added a comment.

use getFailureKind() + rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision as: ilya-biryukov.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks a lot for driving this to conclusion! Please note that there is 
one small NIT you may want to fix before landing this.
UB definitely explains why we saw the bugs. It's good that we caught it before 
this landed.

In D129531#3992998 , @ayzhao wrote:

> I don't think this should be an issue - `InitListExpr` doesn't take 
> `ArrayFiller` into account in `computeDependence(...)` either: 
> https://github.com/llvm/llvm-project/blob/c8647738cd654d9ecfdc047e480d05a997d3127b/clang/lib/AST/ComputeDependence.cpp#L636-L641

Yeah, makes sense, if `InitListExpr` does not take into account we are probably 
good here as well.
My intuition is that we can only figure out what to set in `ArrayFiller` when 
the expressions are non-dependent, therefore the filler itself should not add 
any extra "dependence" to the expression.
If that's the case, it would be nice to add an assertion. But that's unrelated 
to this change, we can do it some other day.




Comment at: clang/lib/Sema/SemaInit.cpp:6199
+  S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() &&
+  Failure == FK_ConstructorOverloadFailed &&
+  onlyHasDefaultedCtors(getFailedCandidateSet())) {

NIT: use `getFailureKind()`, it asserts that `Failed() == true` and would have 
catched this particular UB.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added a comment.

In D129531#3990050 , @ayzhao wrote:

> In D129531#3988996 , @ilya-biryukov 
> wrote:
>
>> Since the errors only shows up in modular builds, I would look closely for 
>> bugs inside `ASTReader`/`ASTWriter`.
>
> These test failures are a little weird because they seem to alternate between 
> being able to reliably reproduce vs not being able to reproduce at all.
>
> One thing that I did notice is that the tests fail with `RelWithDebInfo` 
> builds, but pass with all of the other builds (`Debug`, `Release`, and 
> `MinSizeRel`). Right now though, I'm unable to reproduce the libc++ failure 
> even with `RelWithDebInfo`.
>
> Incidentally, I just rebased this patch, and it seems like the dr2xx.cpp test 
> 
>  is also failing in a similar manner (fails on `RelWithDebInfo` but passes on 
> other builds).

All of the builds are green now - it turns out that I had to resolve 2 
instances of UB in this patch.

>> Also, it seems that `ArrayFiller` is not taken in to account in 
>> `computeDependence` and maybe it should be. I am not 100% sure, though: if 
>> `ArrayFiller` is only used for non-dependent code, it should not case this 
>> bug. It also does not explain the variation between modular and non-modular 
>> builds

I don't think this should be an issue - `InitListExpr` doesn't take 
`ArrayFiller` into account in `computeDependence(...)` either: 
https://github.com/llvm/llvm-project/blob/c8647738cd654d9ecfdc047e480d05a997d3127b/clang/lib/AST/ComputeDependence.cpp#L636-L641


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 482566.
ayzhao added a comment.

fix memory access issues identified by UBSan


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in a constant expression}}

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added a comment.

In D129531#3988996 , @ilya-biryukov 
wrote:

> Since the errors only shows up in modular builds, I would look closely for 
> bugs inside `ASTReader`/`ASTWriter`. 
> Also, it seems that `ArrayFiller` is not taken in to account in 
> `computeDependence` and maybe it should be. I am not 100% sure, though: if 
> `ArrayFiller` is only used for non-dependent code, it should not case this 
> bug. It also does not explain the variation between modular and non-modular 
> builds.

These test failures are a little weird because they seem to alternate between 
being able to reliably reproduce vs not being able to reproduce at all.

One thing that I did notice is that the tests fail with `RelWithDebInfo` 
builds, but pass with all of the other builds (`Debug`, `Release`, and 
`MinSizeRel`). Right now though, I'm unable to reproduce the libc++ failure 
even with `RelWithDebInfo`.

Incidentally, I just rebased this patch, and it seems like the dr2xx.cpp test 
 
is also failing in a similar manner (fails on `RelWithDebInfo` but passes on 
other builds).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 482261.
ayzhao added a comment.

rebase + some compiler warning fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in a constant expression}}
+};
+

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Since the errors only shows up in modular builds, I would look closely for bugs 
inside `ASTReader`/`ASTWriter`. 
Also, it seems that `ArrayFiller` is not taken in to account in 
`computeDependence` and maybe it should be. This assumption is wrong if 
`ArrayFiller` is only used for non-dependent code and it also does not explain 
the variation between modular and non-modular builds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This LG, but not accepting as I believe libc++ is still broken if built with 
modules, right?
I have run `check-cxx` locally and it seems to work, but I suspect that's not 
using modules by default. I have clicked through the links in phrabricator and 
the errors seem to come from the latest uploaded diff.
Let me know if I'm missing something.




Comment at: clang/lib/Sema/SemaInit.cpp:5380
+  }
+  InitExprs.push_back(ER.get());
+}

ayzhao wrote:
> ilya-biryukov wrote:
> > ayzhao wrote:
> > > ayzhao wrote:
> > > > So the libc++ test compile failures are due to this line.
> > > > 
> > > > One example of a failing unit test is 
> > > > [range.take.while/ctor.view.pass](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp).
> > > >  Clang calls this function twice in `TreeTransform.h` - once with 
> > > > `VerifyOnly` set to `true`, once with it set to `false`.
> > > > 
> > > > For some reason, when this function tries to value-initialize the 
> > > > member `MoveOnly mo` in `View`, `Seq.Failed()` returns false after 
> > > > `TryValueInitialization(...)`, but the resulting `ExprResult` is 
> > > > `nullptr`, causing the segfault we see when we push `nullptr` to 
> > > > `InitExprs` and pass `InitExprs` to the constructor of 
> > > > `CXXParenListInitExpr`. One way to be fix this is to move the line 
> > > > `ExprResult ER = Seq.Perform(...)` out of the `if (!VerifyOnly)` block 
> > > > and check for `ER.isInvalid()` instead of `Seq.Failed()`, but that 
> > > > results in test failures due to excess diagnostic messages in 
> > > > `Seq.Perform(...)`
> > > > 
> > > > I'm still looking into this, but if anyone has any ideas, they would be 
> > > > very welcome.
> > > > 
> > > > To repro the buildbot failures, just build clang with this patch, and 
> > > > then in a separate build directory, build the target `check-cxx` using 
> > > > the previously built clang.
> > > I was able to get the above workaround to pass the test by clearing the 
> > > diagnostics after calling `Seq.Perform(...)`.
> > > 
> > > IMO, this should be OK for now, but I'm open to better ideas if anyone 
> > > has any.
> > Clearing all the diagnostics is a nuclear options and definitely seems off 
> > here. We should not call `Perform()` when `VerifyOnly` is `true` to avoid 
> > producing the diagnostics in the first place.
> > 
> > It's fine for the call with `VerifyOnly = true` to return no errors and 
> > later produce diagnostics with `VerifyOnly = false`, I believe this is what 
> > `InitListChecker` is already doing.
> > I have been playing around with the old version of the code, but couldn't 
> > fix it fully. I do have a small example that breaks, we should add it to 
> > the test and it should also be easier to understand what's going on:
> > 
> > ```
> > struct MoveOnly
> > {
> >   MoveOnly(int data = 1);
> >   MoveOnly(const MoveOnly&) = delete;
> >   MoveOnly(MoveOnly&&) = default;
> > };
> > 
> > struct View {
> >   int a;
> >   MoveOnly mo;
> > };
> > 
> > void test() {
> >   View{0};
> >   View(0); // should work, but crashes and produces invalid diagnostics.
> > }
> > ```
> > 
> > In general, my approach would be to try mimicing what `InitListChecker` is 
> > doing as much as possible, trimming all the unnecessary complexity that 
> > braced-init-lists entail.
> > Hope it's helpful.
> So it looks like all I had to do was remove the call to 
> `TryValueInitialization(...)` and just check for `Seq.Failed()`. This is also 
> what we do in `InitListChecker`.
> 
> The `check-cxx` target appears to work for me locally, so fingers crossed 
> that the build passes.
> 
> >  I do have a small example that breaks, we should add it to the test and it 
> > should also be easier to understand what's going on:
> 
> Done.
Good catch! I have also noticed that `TryValueInitialization` is missing in 
`InitListChecker`, but somehow thought it's just doing the same thing without 
reusing the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:5380
+  }
+  InitExprs.push_back(ER.get());
+}

ilya-biryukov wrote:
> ayzhao wrote:
> > ayzhao wrote:
> > > So the libc++ test compile failures are due to this line.
> > > 
> > > One example of a failing unit test is 
> > > [range.take.while/ctor.view.pass](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp).
> > >  Clang calls this function twice in `TreeTransform.h` - once with 
> > > `VerifyOnly` set to `true`, once with it set to `false`.
> > > 
> > > For some reason, when this function tries to value-initialize the member 
> > > `MoveOnly mo` in `View`, `Seq.Failed()` returns false after 
> > > `TryValueInitialization(...)`, but the resulting `ExprResult` is 
> > > `nullptr`, causing the segfault we see when we push `nullptr` to 
> > > `InitExprs` and pass `InitExprs` to the constructor of 
> > > `CXXParenListInitExpr`. One way to be fix this is to move the line 
> > > `ExprResult ER = Seq.Perform(...)` out of the `if (!VerifyOnly)` block 
> > > and check for `ER.isInvalid()` instead of `Seq.Failed()`, but that 
> > > results in test failures due to excess diagnostic messages in 
> > > `Seq.Perform(...)`
> > > 
> > > I'm still looking into this, but if anyone has any ideas, they would be 
> > > very welcome.
> > > 
> > > To repro the buildbot failures, just build clang with this patch, and 
> > > then in a separate build directory, build the target `check-cxx` using 
> > > the previously built clang.
> > I was able to get the above workaround to pass the test by clearing the 
> > diagnostics after calling `Seq.Perform(...)`.
> > 
> > IMO, this should be OK for now, but I'm open to better ideas if anyone has 
> > any.
> Clearing all the diagnostics is a nuclear options and definitely seems off 
> here. We should not call `Perform()` when `VerifyOnly` is `true` to avoid 
> producing the diagnostics in the first place.
> 
> It's fine for the call with `VerifyOnly = true` to return no errors and later 
> produce diagnostics with `VerifyOnly = false`, I believe this is what 
> `InitListChecker` is already doing.
> I have been playing around with the old version of the code, but couldn't fix 
> it fully. I do have a small example that breaks, we should add it to the test 
> and it should also be easier to understand what's going on:
> 
> ```
> struct MoveOnly
> {
>   MoveOnly(int data = 1);
>   MoveOnly(const MoveOnly&) = delete;
>   MoveOnly(MoveOnly&&) = default;
> };
> 
> struct View {
>   int a;
>   MoveOnly mo;
> };
> 
> void test() {
>   View{0};
>   View(0); // should work, but crashes and produces invalid diagnostics.
> }
> ```
> 
> In general, my approach would be to try mimicing what `InitListChecker` is 
> doing as much as possible, trimming all the unnecessary complexity that 
> braced-init-lists entail.
> Hope it's helpful.
So it looks like all I had to do was remove the call to 
`TryValueInitialization(...)` and just check for `Seq.Failed()`. This is also 
what we do in `InitListChecker`.

The `check-cxx` target appears to work for me locally, so fingers crossed that 
the build passes.

>  I do have a small example that breaks, we should add it to the test and it 
> should also be easier to understand what's going on:

Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 481700.
ayzhao marked 6 inline comments as done.
ayzhao added a comment.

address comments + rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in a 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:4824
+  const Expr *getArrayFiller() const {
+return const_cast(this)->getArrayFiller();
+  }

NIT: maybe just use `return ArrayFiller`? The code is much simpler and since 
functions are very close it's pretty much impossible that anyone would refactor 
one of those without looking at the other.



Comment at: clang/lib/AST/ExprConstant.cpp:1
 ImplicitValueInitExpr VIE(Field->getType());
-const Expr *InitExpr = E->getNumInits() ? E->getInit(0) : 
+const Expr *InitExpr = Args.size() ? Args[0] : 
 

NIT: `!Args.empty() ? Args[0] : `
`Args.size()` also works, but arguably captures the intent of the code a bit 
less clearly.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

ayzhao wrote:
> ayzhao wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > ayzhao wrote:
> > > > > ilya-biryukov wrote:
> > > > > > - Should we have a filler for `CXXParenInitListExpr` too?
> > > > > >   It seems like an important optimization and could have large 
> > > > > > effect on compile times if we don't 
> > > > > > 
> > > > > > - Same question for semantic and syntactic-form (similar to 
> > > > > > `InitListExpr`): should we have it here?
> > > > > >   I am not sure if it's semantically required (probably not?), but 
> > > > > > that's definitely something that `clang-tidy` and other source 
> > > > > > tools will rely on.
> > > > > > 
> > > > > > We should probably share the code there, I suggest moving it to a 
> > > > > > shared base class and using it where appropriate instead of the 
> > > > > > derived classes.
> > > > > > Should we have a filler for CXXParenInitListExpr too? It seems like 
> > > > > > an important optimization and could have large effect on compile 
> > > > > > times if we don't
> > > > > 
> > > > > This feels like premature optimization - presumably, wouldn't this 
> > > > > only be an issue with extraordinarily large (say, O(1000)) arrays?
> > > > > 
> > > > > > Same question for semantic and syntactic-form (similar to 
> > > > > > InitListExpr): should we have it here? I am not sure if it's 
> > > > > > semantically required (probably not?), but that's definitely 
> > > > > > something that clang-tidy and other source tools will rely on
> > > > > 
> > > > > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > > > > syntatic form would be the enclosing `ParenListExpr`.
> > > > > This feels like premature optimization - presumably, wouldn't this 
> > > > > only be an issue with extraordinarily large (say, O(1000)) arrays?
> > > > Yes, this should only happen with large arrays. Normally I would agree, 
> > > > but it's surprising that changing `{}` to `()` in the compiler would 
> > > > lead to performance degradation.
> > > > In that sense, this premature optimization is already implemented, we 
> > > > are rather degrading performance for a different syntax to do the same 
> > > > thing.
> > > > 
> > > > Although we could also land without it, but in that case could you open 
> > > > a GH issue and add a FIXME to track the implementation of this 
> > > > particular optimization?
> > > > This should increase the chances of users finding the root cause of the 
> > > > issue if they happen to hit it.
> > > > 
> > > > > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > > > > syntatic form would be the enclosing ParenListExpr.
> > > > Do we even have the enclosing `ParenListExpr`? If I dump the AST with 
> > > > `clang -fsyntax-only -Xclang=-ast-dump ...`  for the following code:
> > > > ```
> > > > struct pair { int a; int b = 2; };
> > > > int main() {
> > > >   pair(1); pair p(1);
> > > >   pair b{1}; pair{1};
> > > > }
> > > > ```
> > > > 
> > > > I get 
> > > > ```
> > > > `-FunctionDecl 0x557d79717e98  line:2:5 main 'int 
> > > > ()'
> > > >   `-CompoundStmt 0x557d797369d0 
> > > > |-CXXFunctionalCastExpr 0x557d79718528  
> > > > 'pair':'pair' functional cast to pair 
> > > > | `-CXXParenListInitExpr 0x557d79718500  'pair':'pair'
> > > > |   |-IntegerLiteral 0x557d79718010  'int' 1
> > > > |   `-IntegerLiteral 0x557d79717e18  'int' 2
> > > > |-DeclStmt 0x557d79718650 
> > > > | `-VarDecl 0x557d79718568  col:17 p 'pair':'pair' 
> > > > parenlistinit
> > > > |   `-CXXParenListInitExpr 0x557d79718610  'pair':'pair'
> > > > | |-IntegerLiteral 0x557d797185d0  'int' 1
> > > > | `-IntegerLiteral 0x557d79717e18  'int' 2
> > > > |-DeclStmt 0x557d797187d8 
> > > > | `-VarDecl 0x557d79718680  col:8 b 'pair':'pair' 
> > > > listinit
> > > > |   `-InitListExpr 0x557d79718750  'pair':'pair'
> > > > | |-IntegerLiteral 0x557d797186e8  'int' 1
> > > > | `-CXXDefaultInitExpr 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
 case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+case VarDecl::ParenListInit:
+  JOS.attribute("init", "paren-list");

ilya-biryukov wrote:
> ayzhao wrote:
> > ilya-biryukov wrote:
> > > NIT: maybe use the same formatting as other switch cases for constistency?
> > Unfortunately clang-format insists that these be on separate lines.
> Ah, that's unfortunate. I normally just revert the effect of `clang-format` 
> for those lines, but up to you.
> Also fine to keep as is or even format the other lines according to the style 
> guide (it's just 3 more lines, so should not be a big deal).
> 
> 
Yeah, I would definitely prefer to keep the current style. The problem though 
is that the buildbots run clang format and will fail if the patch isn't 
formatted correctly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:5380
+  }
+  InitExprs.push_back(ER.get());
+}

ayzhao wrote:
> So the libc++ test compile failures are due to this line.
> 
> One example of a failing unit test is 
> [range.take.while/ctor.view.pass](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp).
>  Clang calls this function twice in `TreeTransform.h` - once with 
> `VerifyOnly` set to `true`, once with it set to `false`.
> 
> For some reason, when this function tries to value-initialize the member 
> `MoveOnly mo` in `View`, `Seq.Failed()` returns false after 
> `TryValueInitialization(...)`, but the resulting `ExprResult` is `nullptr`, 
> causing the segfault we see when we push `nullptr` to `InitExprs` and pass 
> `InitExprs` to the constructor of `CXXParenListInitExpr`. One way to be fix 
> this is to move the line `ExprResult ER = Seq.Perform(...)` out of the `if 
> (!VerifyOnly)` block and check for `ER.isInvalid()` instead of 
> `Seq.Failed()`, but that results in test failures due to excess diagnostic 
> messages in `Seq.Perform(...)`
> 
> I'm still looking into this, but if anyone has any ideas, they would be very 
> welcome.
> 
> To repro the buildbot failures, just build clang with this patch, and then in 
> a separate build directory, build the target `check-cxx` using the previously 
> built clang.
I was able to get the above workaround to pass the test by clearing the 
diagnostics after calling `Seq.Perform(...)`.

IMO, this should be OK for now, but I'm open to better ideas if anyone has any.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 481508.
ayzhao added a comment.

rebase + (hopefully) fix libc++ c++2b test failures


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in a constant 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:5380
+  }
+  InitExprs.push_back(ER.get());
+}

So the libc++ test compile failures are due to this line.

One example of a failing unit test is 
[range.take.while/ctor.view.pass](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp).
 Clang calls this function twice in `TreeTransform.h` - once with `VerifyOnly` 
set to `true`, once with it set to `false`.

For some reason, when this function tries to value-initialize the member 
`MoveOnly mo` in `View`, `Seq.Failed()` returns false after 
`TryValueInitialization(...)`, but the resulting `ExprResult` is `nullptr`, 
causing the segfault we see when we push `nullptr` to `InitExprs` and pass 
`InitExprs` to the constructor of `CXXParenListInitExpr`. One way to be fix 
this is to move the line `ExprResult ER = Seq.Perform(...)` out of the `if 
(!VerifyOnly)` block and check for `ER.isInvalid()` instead of `Seq.Failed()`, 
but that results in test failures due to excess diagnostic messages in 
`Seq.Perform(...)`

I'm still looking into this, but if anyone has any ideas, they would be very 
welcome.

To repro the buildbot failures, just build clang with this patch, and then in a 
separate build directory, build the target `check-cxx` using the previously 
built clang.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/test/CXX/class/class.compare/class.spaceship/p1.cpp:106
   Cmp() <=> Cmp(), // expected-note-re {{in defaulted three-way 
comparison operator for '{{.*}}Cmp<{{.*}}G2>' first required here}}j
-  // expected-error@#cmp {{no matching conversion for static_cast from 
'void' to 'std::strong_ordering'}}
+  // expected-error@#cmp {{static_cast from 'void' to 
'std::strong_ordering' is not allowed}}
   Cmp() <=> Cmp(), // expected-note-re {{in defaulted three-way 
comparison operator for '{{.*}}Cmp<{{.*}}H>' first required here}}j

ilya-biryukov wrote:
> Do you have any idea why did this diagnostic change?
Fixed - I needed to add a line to `SemaCast.cpp`



Comment at: clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp:131
+// pre20-error@-2 {{no matching constructor for initialization of 'B'}}
+// post20-error@-3 {{excess elements in struct initializer}}
+// post20-error@-4 {{excess elements in struct initializer}}

ilya-biryukov wrote:
> Given how pervasive this error is right now, I feel that we want to add a 
> name of the struct to this message.
> This case is also a good example of how this diagnostic can be really low 
> quality with templates: it's unclear which exact base class causes a problem 
> here from the compiler output,.
> 
> Maybe open a GH issue for that? It seems like an independent task that will 
> also affect braced initializers and may need test file updates.
Created https://github.com/llvm/llvm-project/issues/59367


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 480596.
ayzhao marked 3 inline comments as done.
ayzhao added a comment.

restore original diagnostics


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 5{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 480189.
ayzhao marked 2 inline comments as done.
ayzhao added a comment.

add missing EOF newline


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

ayzhao wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > ayzhao wrote:
> > > > ilya-biryukov wrote:
> > > > > - Should we have a filler for `CXXParenInitListExpr` too?
> > > > >   It seems like an important optimization and could have large effect 
> > > > > on compile times if we don't 
> > > > > 
> > > > > - Same question for semantic and syntactic-form (similar to 
> > > > > `InitListExpr`): should we have it here?
> > > > >   I am not sure if it's semantically required (probably not?), but 
> > > > > that's definitely something that `clang-tidy` and other source tools 
> > > > > will rely on.
> > > > > 
> > > > > We should probably share the code there, I suggest moving it to a 
> > > > > shared base class and using it where appropriate instead of the 
> > > > > derived classes.
> > > > > Should we have a filler for CXXParenInitListExpr too? It seems like 
> > > > > an important optimization and could have large effect on compile 
> > > > > times if we don't
> > > > 
> > > > This feels like premature optimization - presumably, wouldn't this only 
> > > > be an issue with extraordinarily large (say, O(1000)) arrays?
> > > > 
> > > > > Same question for semantic and syntactic-form (similar to 
> > > > > InitListExpr): should we have it here? I am not sure if it's 
> > > > > semantically required (probably not?), but that's definitely 
> > > > > something that clang-tidy and other source tools will rely on
> > > > 
> > > > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > > > syntatic form would be the enclosing `ParenListExpr`.
> > > > This feels like premature optimization - presumably, wouldn't this only 
> > > > be an issue with extraordinarily large (say, O(1000)) arrays?
> > > Yes, this should only happen with large arrays. Normally I would agree, 
> > > but it's surprising that changing `{}` to `()` in the compiler would lead 
> > > to performance degradation.
> > > In that sense, this premature optimization is already implemented, we are 
> > > rather degrading performance for a different syntax to do the same thing.
> > > 
> > > Although we could also land without it, but in that case could you open a 
> > > GH issue and add a FIXME to track the implementation of this particular 
> > > optimization?
> > > This should increase the chances of users finding the root cause of the 
> > > issue if they happen to hit it.
> > > 
> > > > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > > > syntatic form would be the enclosing ParenListExpr.
> > > Do we even have the enclosing `ParenListExpr`? If I dump the AST with 
> > > `clang -fsyntax-only -Xclang=-ast-dump ...`  for the following code:
> > > ```
> > > struct pair { int a; int b = 2; };
> > > int main() {
> > >   pair(1); pair p(1);
> > >   pair b{1}; pair{1};
> > > }
> > > ```
> > > 
> > > I get 
> > > ```
> > > `-FunctionDecl 0x557d79717e98  line:2:5 main 'int ()'
> > >   `-CompoundStmt 0x557d797369d0 
> > > |-CXXFunctionalCastExpr 0x557d79718528  
> > > 'pair':'pair' functional cast to pair 
> > > | `-CXXParenListInitExpr 0x557d79718500  'pair':'pair'
> > > |   |-IntegerLiteral 0x557d79718010  'int' 1
> > > |   `-IntegerLiteral 0x557d79717e18  'int' 2
> > > |-DeclStmt 0x557d79718650 
> > > | `-VarDecl 0x557d79718568  col:17 p 'pair':'pair' 
> > > parenlistinit
> > > |   `-CXXParenListInitExpr 0x557d79718610  'pair':'pair'
> > > | |-IntegerLiteral 0x557d797185d0  'int' 1
> > > | `-IntegerLiteral 0x557d79717e18  'int' 2
> > > |-DeclStmt 0x557d797187d8 
> > > | `-VarDecl 0x557d79718680  col:8 b 'pair':'pair' 
> > > listinit
> > > |   `-InitListExpr 0x557d79718750  'pair':'pair'
> > > | |-IntegerLiteral 0x557d797186e8  'int' 1
> > > | `-CXXDefaultInitExpr 0x557d797187a0  'int'
> > > `-CXXFunctionalCastExpr 0x557d797369a8  'pair':'pair' 
> > > functional cast to pair 
> > >   `-InitListExpr 0x557d79718868  'pair':'pair'
> > > |-IntegerLiteral 0x557d79718800  'int' 1
> > > `-CXXDefaultInitExpr 0x557d797188b8  'int'
> > > ```
> > > It feels like the `ParentListExpr` is replaced during semantic analysis 
> > > and there is no way to get it back. I also tried running `clang-query` 
> > > and trying to `match parenListExpr()` and go 0 results. Is it just 
> > > missing in the AST dump and I run `clang-query` incorrectly or do we 
> > > actually not have the syntactic form of this expression after all?
> > Another important thing to address from the dump: notice how braced 
> > initialization creates `CXXDefaultInitExpr` and `CXXParenListInitExpr` 
> > copies the default argument expression directly. It's really important to 
> > use the former form, here's the example that currently breaks:

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 480184.
ayzhao added a comment.

use NumUserSpecifiedExprs instead of syntatic/semantic forms, also address some 
comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao marked 4 inline comments as done.
ayzhao added a comment.

In D129531#3971392 , @ilya-biryukov 
wrote:

> Thanks!
>
> I have two major comments and also inline NITs. Not sure if we should block 
> on those, just wanted to hear your opinions:
>
> - `InitListExpr` and `CXXParenInitListExpr` have some code in common. Code 
> duplication is substantial and I think sharing the common implementation is 
> warranted. E.g. if some code would want to change something with 
> `ArrayFiller` or make a use of it, the work will probably need to be 
> duplicated. To reiterate what I mentioned earlier, I believe deriving 
> `CXXParenInitListExpr` from `InitListExpr` is not a very good option, but we 
> might explore other possibilities: a base class or factoring out common code 
> in a separate struct. I believe this could be done with a follow-up change, 
> though, as the amount of changes required does not seem too big, I would be 
> happy with first landing this patch and then cleaning up with a follow-up 
> change.
> - Did we explore the possibility of modelling this differently and somehow 
> avoiding making `CXXParenInitListExpr` an expression. This seems to borrow 
> from `InitListExpr`, but maybe for the wrong reasons.  Braced initializers 
> can actually occur in expressions positions, e.g. `int a = {123}`. However, 
> `CXXParenInistListExpr` cannot occur in expression positions outside 
> initializations, e.g. having `CXXFunctionalCastExpr` for 
> `aggregate_struct(123)` feels somewhat wrong to me and seems to lead to 
> confusing error messages too. Maybe we should model it as a new expression 
> that contains both the type and the argument list? I.e. more similar to 
> `CXXConstructExpr`?

As discussed offline, let's keep the current implementation as is for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks!

I have two major comments and also inline NITs. Not sure if we should block on 
those, just wanted to hear your opinions:

- `InitListExpr` and `CXXParenInitListExpr` have some code in common. Code 
duplication is substantial and I think sharing the common implementation is 
warranted. E.g. if some code would want to change something with `ArrayFiller` 
or make a use of it, the work will probably need to be duplicated. To reiterate 
what I mentioned earlier, I believe deriving `CXXParenInitListExpr` from 
`InitListExpr` is not a very good option, but we might explore other 
possibilities: a base class or factoring out common code in a separate struct. 
I believe this could be done with a follow-up change, though, as the amount of 
changes required does not seem too big, I would be happy with first landing 
this patch and then cleaning up with a follow-up change.
- Did we explore the possibility of modelling this differently and somehow 
avoiding making `CXXParenInitListExpr` an expression. This seems to borrow from 
`InitListExpr`, but maybe for the wrong reasons.  Braced initializers can 
actually occur in expressions positions, e.g. `int a = {123}`. However, 
`CXXParenInistListExpr` cannot occur in expression positions outside 
initializations, e.g. having `CXXFunctionalCastExpr` for 
`aggregate_struct(123)` feels somewhat wrong to me and seems to lead to 
confusing error messages too. Maybe we should model it as a new expression that 
contains both the type and the argument list? I.e. more similar to 
`CXXConstructExpr`?




Comment at: clang/lib/Sema/SemaInit.cpp:5455
+  // All of the initialized entities are explicitly initialized by
+  // expressionse in the CXXParenListInitExpr. The semantic form is the
+  // same as the syntatic form

NIT: s/expressionse/expressions



Comment at: clang/lib/Sema/SemaInit.cpp:9803
+TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
+  /*VerifyOnly=*/false);
   }

NIT: add break 



Comment at: clang/lib/Sema/TreeTransform.h:14040
+  ArrayRef InitExprs = E->getInitExprs();
+  if (TransformExprs(reinterpret_cast(InitExprs.data()),
+ InitExprs.size(), true, TransformedInits))

`reinterpret_cast` seems redundant here or am I missing something?



Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2172
+  VisitExpr(E);
+  unsigned expectedNumExprs = Record.readInt();
+  assert(E->NumExprs == expectedNumExprs &&

NIT: `ExpectedNumExprs`




Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2181
+E->getTrailingObjects()[I] = Record.readSubExpr();
+  E->updateDependence();
+

NIT: maybe add a comment that dependence does not depend on filler or syntactic 
form. This might be non-obvious for someone who did not look into the 
implementation.

Alternatively, you could run this at the end of this function, it should not be 
confusing to anyone that certain properties get computed *after* the expression 
was fully deserialized. 



Comment at: clang/test/CXX/class/class.compare/class.spaceship/p1.cpp:106
   Cmp() <=> Cmp(), // expected-note-re {{in defaulted three-way 
comparison operator for '{{.*}}Cmp<{{.*}}G2>' first required here}}j
-  // expected-error@#cmp {{no matching conversion for static_cast from 
'void' to 'std::strong_ordering'}}
+  // expected-error@#cmp {{static_cast from 'void' to 
'std::strong_ordering' is not allowed}}
   Cmp() <=> Cmp(), // expected-note-re {{in defaulted three-way 
comparison operator for '{{.*}}Cmp<{{.*}}H>' first required here}}j

Do you have any idea why did this diagnostic change?



Comment at: clang/test/CXX/drs/dr2xx.cpp:161
+void test6(A::S as) { struct f {}; (void) f(as); } // pre20-error {{no 
matching conversion}} pre20-note +{{}}
+// post20-error@-1 {{functional-style cast from 'A::S' to 'f' is not 
allowed}}
   };

This might be related to other diagnostic change.
Why do we see the new behavior here? Do you think we should keep the old 
behavior maybe? 
I feel that both errors are communicating the same thing and probably keeping 
the current state is acceptable, just want to better understand how we got 
there.

It definitely has something to do with the initialization code, but I struggle 
to understand what exactly changed



Comment at: clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp:131
+// pre20-error@-2 {{no matching constructor for initialization of 'B'}}
+// post20-error@-3 {{excess elements in struct initializer}}
+// post20-error@-4 {{excess elements in struct initializer}}

Given how pervasive this error is right now, I feel that we want to add a name 
of the struct to this message.
This case is also a 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > ayzhao wrote:
> > > ilya-biryukov wrote:
> > > > - Should we have a filler for `CXXParenInitListExpr` too?
> > > >   It seems like an important optimization and could have large effect 
> > > > on compile times if we don't 
> > > > 
> > > > - Same question for semantic and syntactic-form (similar to 
> > > > `InitListExpr`): should we have it here?
> > > >   I am not sure if it's semantically required (probably not?), but 
> > > > that's definitely something that `clang-tidy` and other source tools 
> > > > will rely on.
> > > > 
> > > > We should probably share the code there, I suggest moving it to a 
> > > > shared base class and using it where appropriate instead of the derived 
> > > > classes.
> > > > Should we have a filler for CXXParenInitListExpr too? It seems like an 
> > > > important optimization and could have large effect on compile times if 
> > > > we don't
> > > 
> > > This feels like premature optimization - presumably, wouldn't this only 
> > > be an issue with extraordinarily large (say, O(1000)) arrays?
> > > 
> > > > Same question for semantic and syntactic-form (similar to 
> > > > InitListExpr): should we have it here? I am not sure if it's 
> > > > semantically required (probably not?), but that's definitely something 
> > > > that clang-tidy and other source tools will rely on
> > > 
> > > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > > syntatic form would be the enclosing `ParenListExpr`.
> > > This feels like premature optimization - presumably, wouldn't this only 
> > > be an issue with extraordinarily large (say, O(1000)) arrays?
> > Yes, this should only happen with large arrays. Normally I would agree, but 
> > it's surprising that changing `{}` to `()` in the compiler would lead to 
> > performance degradation.
> > In that sense, this premature optimization is already implemented, we are 
> > rather degrading performance for a different syntax to do the same thing.
> > 
> > Although we could also land without it, but in that case could you open a 
> > GH issue and add a FIXME to track the implementation of this particular 
> > optimization?
> > This should increase the chances of users finding the root cause of the 
> > issue if they happen to hit it.
> > 
> > > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > > syntatic form would be the enclosing ParenListExpr.
> > Do we even have the enclosing `ParenListExpr`? If I dump the AST with 
> > `clang -fsyntax-only -Xclang=-ast-dump ...`  for the following code:
> > ```
> > struct pair { int a; int b = 2; };
> > int main() {
> >   pair(1); pair p(1);
> >   pair b{1}; pair{1};
> > }
> > ```
> > 
> > I get 
> > ```
> > `-FunctionDecl 0x557d79717e98  line:2:5 main 'int ()'
> >   `-CompoundStmt 0x557d797369d0 
> > |-CXXFunctionalCastExpr 0x557d79718528  'pair':'pair' 
> > functional cast to pair 
> > | `-CXXParenListInitExpr 0x557d79718500  'pair':'pair'
> > |   |-IntegerLiteral 0x557d79718010  'int' 1
> > |   `-IntegerLiteral 0x557d79717e18  'int' 2
> > |-DeclStmt 0x557d79718650 
> > | `-VarDecl 0x557d79718568  col:17 p 'pair':'pair' 
> > parenlistinit
> > |   `-CXXParenListInitExpr 0x557d79718610  'pair':'pair'
> > | |-IntegerLiteral 0x557d797185d0  'int' 1
> > | `-IntegerLiteral 0x557d79717e18  'int' 2
> > |-DeclStmt 0x557d797187d8 
> > | `-VarDecl 0x557d79718680  col:8 b 'pair':'pair' 
> > listinit
> > |   `-InitListExpr 0x557d79718750  'pair':'pair'
> > | |-IntegerLiteral 0x557d797186e8  'int' 1
> > | `-CXXDefaultInitExpr 0x557d797187a0  'int'
> > `-CXXFunctionalCastExpr 0x557d797369a8  'pair':'pair' 
> > functional cast to pair 
> >   `-InitListExpr 0x557d79718868  'pair':'pair'
> > |-IntegerLiteral 0x557d79718800  'int' 1
> > `-CXXDefaultInitExpr 0x557d797188b8  'int'
> > ```
> > It feels like the `ParentListExpr` is replaced during semantic analysis and 
> > there is no way to get it back. I also tried running `clang-query` and 
> > trying to `match parenListExpr()` and go 0 results. Is it just missing in 
> > the AST dump and I run `clang-query` incorrectly or do we actually not have 
> > the syntactic form of this expression after all?
> Another important thing to address from the dump: notice how braced 
> initialization creates `CXXDefaultInitExpr` and `CXXParenListInitExpr` copies 
> the default argument expression directly. It's really important to use the 
> former form, here's the example that currently breaks:
> 
> 
> ```
> #include 
> 
> struct func_init { int some_int; const char* fn = __builtin_FUNCTION(); };
> 
> int main() {
>   func_init a(10);
>   std::cout << "From paren init:" << a.fn << std::endl;

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 479758.
ayzhao marked 2 inline comments as done.
ayzhao added a comment.

implement CXXDefaultInitExpr and ImplicitValueInitExpr, array fillers, and  
semantic vs syntatic forms.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3006,6 +3007,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5587,6 +5591,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

ilya-biryukov wrote:
> ayzhao wrote:
> > ilya-biryukov wrote:
> > > - Should we have a filler for `CXXParenInitListExpr` too?
> > >   It seems like an important optimization and could have large effect on 
> > > compile times if we don't 
> > > 
> > > - Same question for semantic and syntactic-form (similar to 
> > > `InitListExpr`): should we have it here?
> > >   I am not sure if it's semantically required (probably not?), but that's 
> > > definitely something that `clang-tidy` and other source tools will rely 
> > > on.
> > > 
> > > We should probably share the code there, I suggest moving it to a shared 
> > > base class and using it where appropriate instead of the derived classes.
> > > Should we have a filler for CXXParenInitListExpr too? It seems like an 
> > > important optimization and could have large effect on compile times if we 
> > > don't
> > 
> > This feels like premature optimization - presumably, wouldn't this only be 
> > an issue with extraordinarily large (say, O(1000)) arrays?
> > 
> > > Same question for semantic and syntactic-form (similar to InitListExpr): 
> > > should we have it here? I am not sure if it's semantically required 
> > > (probably not?), but that's definitely something that clang-tidy and 
> > > other source tools will rely on
> > 
> > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > syntatic form would be the enclosing `ParenListExpr`.
> > This feels like premature optimization - presumably, wouldn't this only be 
> > an issue with extraordinarily large (say, O(1000)) arrays?
> Yes, this should only happen with large arrays. Normally I would agree, but 
> it's surprising that changing `{}` to `()` in the compiler would lead to 
> performance degradation.
> In that sense, this premature optimization is already implemented, we are 
> rather degrading performance for a different syntax to do the same thing.
> 
> Although we could also land without it, but in that case could you open a GH 
> issue and add a FIXME to track the implementation of this particular 
> optimization?
> This should increase the chances of users finding the root cause of the issue 
> if they happen to hit it.
> 
> > IIRC this doesn't apply to paren list aggregate expressions, as the 
> > syntatic form would be the enclosing ParenListExpr.
> Do we even have the enclosing `ParenListExpr`? If I dump the AST with `clang 
> -fsyntax-only -Xclang=-ast-dump ...`  for the following code:
> ```
> struct pair { int a; int b = 2; };
> int main() {
>   pair(1); pair p(1);
>   pair b{1}; pair{1};
> }
> ```
> 
> I get 
> ```
> `-FunctionDecl 0x557d79717e98  line:2:5 main 'int ()'
>   `-CompoundStmt 0x557d797369d0 
> |-CXXFunctionalCastExpr 0x557d79718528  'pair':'pair' 
> functional cast to pair 
> | `-CXXParenListInitExpr 0x557d79718500  'pair':'pair'
> |   |-IntegerLiteral 0x557d79718010  'int' 1
> |   `-IntegerLiteral 0x557d79717e18  'int' 2
> |-DeclStmt 0x557d79718650 
> | `-VarDecl 0x557d79718568  col:17 p 'pair':'pair' 
> parenlistinit
> |   `-CXXParenListInitExpr 0x557d79718610  'pair':'pair'
> | |-IntegerLiteral 0x557d797185d0  'int' 1
> | `-IntegerLiteral 0x557d79717e18  'int' 2
> |-DeclStmt 0x557d797187d8 
> | `-VarDecl 0x557d79718680  col:8 b 'pair':'pair' listinit
> |   `-InitListExpr 0x557d79718750  'pair':'pair'
> | |-IntegerLiteral 0x557d797186e8  'int' 1
> | `-CXXDefaultInitExpr 0x557d797187a0  'int'
> `-CXXFunctionalCastExpr 0x557d797369a8  'pair':'pair' 
> functional cast to pair 
>   `-InitListExpr 0x557d79718868  'pair':'pair'
> |-IntegerLiteral 0x557d79718800  'int' 1
> `-CXXDefaultInitExpr 0x557d797188b8  'int'
> ```
> It feels like the `ParentListExpr` is replaced during semantic analysis and 
> there is no way to get it back. I also tried running `clang-query` and trying 
> to `match parenListExpr()` and go 0 results. Is it just missing in the AST 
> dump and I run `clang-query` incorrectly or do we actually not have the 
> syntactic form of this expression after all?
Another important thing to address from the dump: notice how braced 
initialization creates `CXXDefaultInitExpr` and `CXXParenListInitExpr` copies 
the default argument expression directly. It's really important to use the 
former form, here's the example that currently breaks:


```
#include 

struct func_init { int some_int; const char* fn = __builtin_FUNCTION(); };

int main() {
  func_init a(10);
  std::cout << "From paren init:" << a.fn << std::endl;

  func_init b{10};
  std::cout << "From braced init: " << b.fn << std::endl;
}
```

The program above is expected to report `main` for both cases, but instead we 
get:
```
From paren init:
From braced init: main
```

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for addressing the comments and sorry for a wait before the comments.
Please see the comment about syntactic form, I might be holding it wrong, but 
it looks like we actually loose the syntactic form completely in this case.




Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
 case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+case VarDecl::ParenListInit:
+  JOS.attribute("init", "paren-list");

ayzhao wrote:
> ilya-biryukov wrote:
> > NIT: maybe use the same formatting as other switch cases for constistency?
> Unfortunately clang-format insists that these be on separate lines.
Ah, that's unfortunate. I normally just revert the effect of `clang-format` for 
those lines, but up to you.
Also fine to keep as is or even format the other lines according to the style 
guide (it's just 3 more lines, so should not be a big deal).





Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

ayzhao wrote:
> ilya-biryukov wrote:
> > - Should we have a filler for `CXXParenInitListExpr` too?
> >   It seems like an important optimization and could have large effect on 
> > compile times if we don't 
> > 
> > - Same question for semantic and syntactic-form (similar to 
> > `InitListExpr`): should we have it here?
> >   I am not sure if it's semantically required (probably not?), but that's 
> > definitely something that `clang-tidy` and other source tools will rely on.
> > 
> > We should probably share the code there, I suggest moving it to a shared 
> > base class and using it where appropriate instead of the derived classes.
> > Should we have a filler for CXXParenInitListExpr too? It seems like an 
> > important optimization and could have large effect on compile times if we 
> > don't
> 
> This feels like premature optimization - presumably, wouldn't this only be an 
> issue with extraordinarily large (say, O(1000)) arrays?
> 
> > Same question for semantic and syntactic-form (similar to InitListExpr): 
> > should we have it here? I am not sure if it's semantically required 
> > (probably not?), but that's definitely something that clang-tidy and other 
> > source tools will rely on
> 
> IIRC this doesn't apply to paren list aggregate expressions, as the syntatic 
> form would be the enclosing `ParenListExpr`.
> This feels like premature optimization - presumably, wouldn't this only be an 
> issue with extraordinarily large (say, O(1000)) arrays?
Yes, this should only happen with large arrays. Normally I would agree, but 
it's surprising that changing `{}` to `()` in the compiler would lead to 
performance degradation.
In that sense, this premature optimization is already implemented, we are 
rather degrading performance for a different syntax to do the same thing.

Although we could also land without it, but in that case could you open a GH 
issue and add a FIXME to track the implementation of this particular 
optimization?
This should increase the chances of users finding the root cause of the issue 
if they happen to hit it.

> IIRC this doesn't apply to paren list aggregate expressions, as the syntatic 
> form would be the enclosing ParenListExpr.
Do we even have the enclosing `ParenListExpr`? If I dump the AST with `clang 
-fsyntax-only -Xclang=-ast-dump ...`  for the following code:
```
struct pair { int a; int b = 2; };
int main() {
  pair(1); pair p(1);
  pair b{1}; pair{1};
}
```

I get 
```
`-FunctionDecl 0x557d79717e98  line:2:5 main 'int ()'
  `-CompoundStmt 0x557d797369d0 
|-CXXFunctionalCastExpr 0x557d79718528  'pair':'pair' 
functional cast to pair 
| `-CXXParenListInitExpr 0x557d79718500  'pair':'pair'
|   |-IntegerLiteral 0x557d79718010  'int' 1
|   `-IntegerLiteral 0x557d79717e18  'int' 2
|-DeclStmt 0x557d79718650 
| `-VarDecl 0x557d79718568  col:17 p 'pair':'pair' 
parenlistinit
|   `-CXXParenListInitExpr 0x557d79718610  'pair':'pair'
| |-IntegerLiteral 0x557d797185d0  'int' 1
| `-IntegerLiteral 0x557d79717e18  'int' 2
|-DeclStmt 0x557d797187d8 
| `-VarDecl 0x557d79718680  col:8 b 'pair':'pair' listinit
|   `-InitListExpr 0x557d79718750  'pair':'pair'
| |-IntegerLiteral 0x557d797186e8  'int' 1
| `-CXXDefaultInitExpr 0x557d797187a0  'int'
`-CXXFunctionalCastExpr 0x557d797369a8  'pair':'pair' 
functional cast to pair 
  `-InitListExpr 0x557d79718868  'pair':'pair'
|-IntegerLiteral 0x557d79718800  'int' 1
`-CXXDefaultInitExpr 0x557d797188b8  'int'
```
It feels like the `ParentListExpr` is replaced during semantic analysis and 
there is no way to get it back. I also tried running `clang-query` and trying 
to `match parenListExpr()` and go 0 results. Is it just missing in the AST dump 
and I run `clang-query` incorrectly or do we actually not have the syntactic 
form of this expression 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.



Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
 case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+case VarDecl::ParenListInit:
+  JOS.attribute("init", "paren-list");

ilya-biryukov wrote:
> NIT: maybe use the same formatting as other switch cases for constistency?
Unfortunately clang-format insists that these be on separate lines.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

ilya-biryukov wrote:
> - Should we have a filler for `CXXParenInitListExpr` too?
>   It seems like an important optimization and could have large effect on 
> compile times if we don't 
> 
> - Same question for semantic and syntactic-form (similar to `InitListExpr`): 
> should we have it here?
>   I am not sure if it's semantically required (probably not?), but that's 
> definitely something that `clang-tidy` and other source tools will rely on.
> 
> We should probably share the code there, I suggest moving it to a shared base 
> class and using it where appropriate instead of the derived classes.
> Should we have a filler for CXXParenInitListExpr too? It seems like an 
> important optimization and could have large effect on compile times if we 
> don't

This feels like premature optimization - presumably, wouldn't this only be an 
issue with extraordinarily large (say, O(1000)) arrays?

> Same question for semantic and syntactic-form (similar to InitListExpr): 
> should we have it here? I am not sure if it's semantically required (probably 
> not?), but that's definitely something that clang-tidy and other source tools 
> will rely on

IIRC this doesn't apply to paren list aggregate expressions, as the syntatic 
form would be the enclosing `ParenListExpr`.



Comment at: clang/lib/Sema/SemaInit.cpp:6169
+  Failure == FK_ConstructorOverloadFailed &&
+  getFailedCandidateSet().size() == 3) {
+// C++20 [dcl.init] 17.6.2.2:

ilya-biryukov wrote:
> It seems shaky to rely on the size of `getFailedCandidateSet`.
> What are we trying to achieve here? Is there a more direct way to specify it?
Done.

As mentioned in the above comment, we try to parse this as a `CXXParenListInit` 
if the only failed overload constructor candidates are the default constructor, 
the default copy constructor, and the default move constructor).



Comment at: clang/lib/Sema/SemaInit.cpp:6188
+//  (6.7.7), and there is no brace elision.
+TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
+  /*VerifyOnly=*/true);

ilya-biryukov wrote:
> NIT: maybe move the full comment into the body of the function?
> It describes in detail what the body of the function does and it would be 
> easier to understand whether the code fits the intention in the standard.
> Having the first part of the comment here would still be useful, probably.
Done. Most of this comment is spread out throughout the body of the function 
anyways.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 476625.
ayzhao marked 9 inline comments as done.
ayzhao added a comment.

address code review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3000,6 +3001,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5595,6 +5599,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for not reviewing this sooner. This looks very good overall, but I still 
have some NITs and a few major comments. For the major points see:

- the comment about the filler and the syntactic/semantic form for the newly 
added expression,
- the comment about relying on `getFailedCandidateSet().size()`.




Comment at: clang/lib/AST/ExprCXX.cpp:1776
+}
\ No newline at end of file


NIT: add newline



Comment at: clang/lib/AST/ExprConstant.cpp:9988
+  }
+} else
+  llvm_unreachable(

NIT: maybe add braces as the other branches have them? This seems to be in the 
[style 
guide](https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements):
> readability is also harmed if an if/else chain does not use braced bodies for 
> either all or none of its members



Comment at: clang/lib/AST/ExprConstant.cpp:10957
+  unsigned NumElts = InitExprs.size();
+  Result = APValue(APValue::UninitArray(), NumElts, NumElts);
+

Could we add an assertion that the array size and the `InitExprs.size()` are 
the same?
E.g. it can differ in the case of `InitListExpr` (probably due to the filler 
optimization?), it would be nice to add a sanity check here.



Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
 case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+case VarDecl::ParenListInit:
+  JOS.attribute("init", "paren-list");

NIT: maybe use the same formatting as other switch cases for constistency?



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:479
 void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
-   QualType ArrayQTy, InitListExpr *E) {
-  uint64_t NumInitElements = E->getNumInits();
+   QualType ArrayQTy, Expr *ExprToVisit,
+   ArrayRef Args) {

NIT: maybe add a comment that `ExprToVisit` is either `InitListExpr` or 
`CXXParenInitListExpr`?



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast(ExprToVisit))
+filler = ILE->getArrayFiller();

- Should we have a filler for `CXXParenInitListExpr` too?
  It seems like an important optimization and could have large effect on 
compile times if we don't 

- Same question for semantic and syntactic-form (similar to `InitListExpr`): 
should we have it here?
  I am not sure if it's semantically required (probably not?), but that's 
definitely something that `clang-tidy` and other source tools will rely on.

We should probably share the code there, I suggest moving it to a shared base 
class and using it where appropriate instead of the derived classes.



Comment at: clang/lib/Sema/SemaInit.cpp:6169
+  Failure == FK_ConstructorOverloadFailed &&
+  getFailedCandidateSet().size() == 3) {
+// C++20 [dcl.init] 17.6.2.2:

It seems shaky to rely on the size of `getFailedCandidateSet`.
What are we trying to achieve here? Is there a more direct way to specify it?



Comment at: clang/lib/Sema/SemaInit.cpp:6188
+//  (6.7.7), and there is no brace elision.
+TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
+  /*VerifyOnly=*/true);

NIT: maybe move the full comment into the body of the function?
It describes in detail what the body of the function does and it would be 
easier to understand whether the code fits the intention in the standard.
Having the first part of the comment here would still be useful, probably.



Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2174
+  E->NumExprs = Record.readInt();
+  for (unsigned I = 0; I < E->NumExprs; I++)
+E->getTrailingObjects()[I] = Record.readSubExpr();

Could we assign the `CXXParenInitListExpr.NumExprs` on construction and add the 
assertion sanity-checking the numbers here:
```
unsigned NumExprs = Record.readInt();
(void)NumExprs; // to avoid unused warnings in NDEBUG builds.
assert(NumExprs == E->NumExprs);
```

(Looked up how `DesignatedInitExpr` does a similar thing, probably good for 
consistency in addition to added safety)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am in the committee meeting this week but if you ping next week I should have 
time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

There is a C++ standard meeting this week so some folks won't be able to review 
it. Feel free to re-ping next week if nobody else replies.
Because it's a fairly big change and I've been known to miss glaring things, I 
don't feel comfortable to approve it. But it looks great to me.
Either way we need to make sure this ships in Clang 16.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 473789.
ayzhao added a comment.

s/pro20/post20/g


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3000,6 +3001,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5579,6 +5583,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in a constant expression}}
+};
+

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 473779.
ayzhao added a comment.

remove extra parens


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3000,6 +3001,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5579,6 +5583,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used in a constant expression}}
+};
+

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao updated this revision to Diff 473772.
ayzhao added a comment.

whoops, uploaded the wrong commit. should be fixed now


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ComputeDependence.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ComputeDependence.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
  clang/test/CXX/drs/dr2xx.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  clang/test/CodeGen/paren-list-agg-init.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/PCH/cxx_paren_init.cpp
  clang/test/PCH/cxx_paren_init.h
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1156,7 +1156,7 @@
 
   Parenthesized initialization of aggregates
   https://wg21.link/p0960r3;>P0960R3
-  No
+  Clang 16
 

 https://wg21.link/p1975r0;>P1975R0
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -643,6 +643,10 @@
 K = CXCursor_RequiresExpr;
 break;
 
+  case Stmt::CXXParenListInitExprClass:
+K = CXCursor_CXXParenListInitExpr;
+break;
+
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2139,6 +2139,7 @@
   void VisitLambdaExpr(const LambdaExpr *E);
   void VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E);
   void VisitRequiresExpr(const RequiresExpr *E);
+  void VisitCXXParenListInitExpr(const CXXParenListInitExpr *E);
   void VisitOMPExecutableDirective(const OMPExecutableDirective *D);
   void VisitOMPLoopBasedDirective(const OMPLoopBasedDirective *D);
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
@@ -3000,6 +3001,9 @@
   for (ParmVarDecl *VD : E->getLocalParameters())
 AddDecl(VD);
 }
+void EnqueueVisitor::VisitCXXParenListInitExpr(const CXXParenListInitExpr *E) {
+  EnqueueChildren(E);
+}
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
   Visit(E->getSyntacticForm());
@@ -5579,6 +5583,8 @@
 return cxstring::createRef("ConceptSpecializationExpr");
   case CXCursor_RequiresExpr:
 return cxstring::createRef("RequiresExpr");
+  case CXCursor_CXXParenListInitExpr:
+return cxstring::createRef("CXXParenListInitExpr");
   case CXCursor_UnexposedStmt:
 return cxstring::createRef("UnexposedStmt");
   case CXCursor_DeclStmt:
Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -0,0 +1,172 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
+
+struct A { // expected-note 4{{candidate constructor}}
+  char i;
+  double j;
+};
+
+struct B {
+  A a;
+  int b[20];
+  int & // expected-note {{reference member declared here}}
+};
+
+struct C { // expected-note 2{{candidate constructor}}
+  A a;
+  int b[20];
+};
+
+struct D : public C, public A {
+  int a;
+};
+
+struct E { // expected-note 3{{candidate constructor}}
+  struct F {
+F(int, int);
+  };
+  int a;
+  F f;
+};
+
+int getint(); // expected-note {{declared here}}
+
+struct F {
+  int a;
+  int b = getint(); // expected-note {{non-constexpr function 'getint' cannot be used 

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added a comment.

Friendly ping, does anyone else have any more comments?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129531/new/

https://reviews.llvm.org/D129531

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits