[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 434680.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D119646

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -651,6 +651,27 @@
 
 } // namespace value_dependent
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo() {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -19611,6 +19611,12 @@
 Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression
+// will be evaluated and folded to a value.
+return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
 // If we were asked not to visit local variables, don't.
 if (SkipLocalVariables) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2867,7 +2867,8 @@
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();
 
   return Arg;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -166,6 +166,8 @@
 - Allow use of an elaborated type specifier as a ``_Generic`` selection
   association in C++ mode. This fixes
   `Issue 55562 `_.
+- Clang will allow calling a ``consteval`` function in a default argument. This
+  fixes `Issue 48230 `_.
 
 Improvements to Clang's diagnostics
 ^^^


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -651,6 +651,27 @@
 
 } // namespace value_dependent
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo() {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -19611,6 +19611,12 @@
 Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression
+// will be evaluated and folded to a value.
+return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
 // If we were asked not to visit local variables, don't.
 if (SkipLocalVariables) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2867,7 +2867,8 @@
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();
 
   return Arg;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -166,6 +166,8 @@
 - Allow use of an elaborated type specifier as a ``_Generic`` selection
   association in C++ mode. This fixes
   `Issue 55562 `_.
+- Clang will allow calling a ``consteval`` function in a default argument. This
+  fixes `Issue 48230 `_.
 
 Improvements to Clang's diagnostics
 ^^^

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM with some small fixes to the release note. Thanks @ChuanqiXu!




Comment at: clang/docs/ReleaseNotes.rst:166
   type.
+- Clang will allow constexpr function in default argument. This fixes
+  `Issue 48230 `_.

Minor corrections


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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 434373.
ChuanqiXu added a comment.

Add release notes.


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

https://reviews.llvm.org/D119646

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -651,6 +651,27 @@
 
 } // namespace value_dependent
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo() {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -19611,6 +19611,12 @@
 Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression
+// will be evaluated and folded to a value.
+return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
 // If we were asked not to visit local variables, don't.
 if (SkipLocalVariables) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2867,7 +2867,8 @@
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();
 
   return Arg;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -163,6 +163,8 @@
 - Unscoped and scoped enumeration types can no longer be initialized from a
   brace-init-list containing a single element of a different scoped enumeration
   type.
+- Clang will allow constexpr function in default argument. This fixes
+  `Issue 48230 `_.
 
 Improvements to Clang's diagnostics
 ^^^


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -651,6 +651,27 @@
 
 } // namespace value_dependent
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo() {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -19611,6 +19611,12 @@
 Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression
+// will be evaluated and folded to a value.
+return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
 // If we were asked not to visit local variables, don't.
 if (SkipLocalVariables) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2867,7 +2867,8 @@
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();
 
   return Arg;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -163,6 +163,8 @@
 - Unscoped and scoped enumeration types can no longer be initialized from a
   brace-init-list containing a single element of a different scoped enumeration
   type.
+- Clang will allow constexpr function in default argument. This fixes
+  `Issue 48230 `_.
 
 Improvements to Clang's diagnostics
 ^^^
___
cfe-commits mailing list

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu commandeered this revision.
ChuanqiXu edited reviewers, added: Izaron; removed: ChuanqiXu.
ChuanqiXu added a comment.

Given this is close to complete and @Izaron is not active recently, let me take 
this over. The authority would be remained, of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM as well, but please add a release note about the bug fix (and reference 
the issue it closes).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

This seems innocuous enough, and seems to better reflect the intent of the 
standard, so LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Chuanqi Xu via Phabricator via cfe-commits]

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

This change LGTM and I prefer the change than D74130 
 since this one is much more comprehensible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

In D119646#3317473 , @cor3ntin wrote:

> There is also https://reviews.llvm.org/D74130 which tries to address the same 
> issue in a very different way.
> I don't really understand the different approaches yet.

Thanks! I spent some time researching this review (but tbh I don't fully 
understand it yet too). I can say why this patch seems to be so different.

The author aims to make this snippet working

  consteval int f1() { return 0; }
  consteval auto g() { return f1; }
  consteval int h(int (*p)() = g()) { return p(); } // should work

That is, they want to make usable pointers to consteval function in default 
arguments. My patch doesn't compile this code now (the behaviour is same as in 
trunk).
As far as I see, the main problem is that the evaluation of ParmVarDecl's 
ConstantExpr is "isolated" from the evaluation of the consteval function body. 
Therefore the pointer "breaks the constexpr wall" and "falls out" to run-time, 
that's illegal. I'm not sure if my explanation is clear...

I wonder if it could be a different pull request later if someone will want to 
make pointers working? The review you mentioned is 2 years old, pretty massive 
and still has failing tests.




Comment at: clang/lib/AST/Decl.cpp:2816
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();

erichkeane wrote:
> So what is happening here?  I would still expect us to give the proper 
> default-arg in this case?  What subtly am I missing?
If we have nested ConstantExprs within the default argument expression, like 
here
```
consteval int Fun(int x) { return x; }

struct Test {
  Test(int loc = Fun(1) * 3 + Fun(2)) {}
  // or, for example, Test(int loc = 0 + Fun(4));
};
```
Then the patch fixes it fine with just `void VisitConstantExpr...` (the snippet 
where you said //"This part makes sense to me."//).

The AST for ParmVarDecl looks like this - [[ 
https://gist.githubusercontent.com/Izaron/7b49d4814a04d16c8014d973bd397ac4/raw/fd5948ca164a381ed13950f04934cb19f847141d/nested.ast
 | snippet on gist.github.com ]]. Clang will fold the ConstantExprs.

---

However, if the ConstantExpr is top-level, that is, the code looks like this:
```
struct Test {
  Test(int loc = Fun(4)) {}
};
```
Then the AST looks like this - [[ 
https://gist.githubusercontent.com/Izaron/dec1f3f0b62769c8fe4f4cb961c211d7/raw/79c5b9364e93de7ebc3d905f87162e000c6e695b/top-level.ast
 | snippet on gist.github.com ]].

There are some places in the code where we call `ParmVarDecl::getDefaultArg()` 
(CodeGen/etc.) The callee will "jump over" the ConstantExpr (since it is 
derived from FullExpr) and give you the nested CallExpr. That's quite incorrent 
because we aren't going to evaluate this CallExpr in run-time. For example, if 
we don't patch this place, the CodeGen will declare `Fun` at LLVM IR, and 
everything breaks.

So I decided that we shouldn't "jump over" ConstantExpr, otherwise it 
incorrectly says to the caller that we are supposed to calculate it in run-time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

There is also https://reviews.llvm.org/D74130 which tries to address the same 
issue in a very different way.
I don't really understand the different approaches yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/Decl.cpp:2816
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();

So what is happening here?  I would still expect us to give the proper 
default-arg in this case?  What subtly am I missing?



Comment at: clang/lib/Sema/SemaExpr.cpp:18967
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression

This part makes sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119646

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

[PATCH] D119646: [clang] Allow consteval functions in default arguments

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron created this revision.
Izaron added reviewers: aaron.ballman, erichkeane, andreasfertig.
Herald added a subscriber: kristof.beyls.
Izaron requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We should not mark a function as "referenced" if we call it within a
ConstantExpr, because the expression will be folded to a value in LLVM IR.
To prevent emitting consteval function declarations, we should not "jump over"
a ConstantExpr when it is a top-level ParmVarDecl's subexpression.

Fixes https://github.com/llvm/llvm-project/issues/48230


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119646

Files:
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,27 @@
 
 } // namespace unevaluated
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo() {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -18964,6 +18964,12 @@
 Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression
+// will be evaluated and folded to a value.
+return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
 // If we were asked not to visit local variables, don't.
 if (SkipLocalVariables) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2813,7 +2813,8 @@
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();
 
   return Arg;
 }


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,27 @@
 
 } // namespace unevaluated
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo() {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -18964,6 +18964,12 @@
 Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+// Don't mark declarations within a ConstantExpression, as this expression
+// will be evaluated and folded to a value.
+return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
 // If we were asked not to visit local variables, don't.
 if (SkipLocalVariables) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2813,7 +2813,8 @@
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null(Arg))
-return E->getSubExpr();
+if (!isa(E))
+  return E->getSubExpr();
 
   return Arg;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits