[PATCH] D132398: Allow constant static members to be used with 'this'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D132398#4150851 , @cor3ntin wrote:

> Given this has not made progress in a while, I think we should try to 
> implement 
> https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2280r4.html instead 
> of this approach, as I'm not sure if that's strictly conforming, and a more 
> holistic approach is likely to lead to better results.
> What do people think?

P2280R4 was adopted for C++23, so I think we'll need to do the work eventually. 
It likely makes sense to explore doing that work now, at the very least so we 
don't make it harder to implement in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132398

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

[PATCH] D132398: Allow constant static members to be used with 'this'

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.
Herald added a subscriber: ChuanqiXu.

Given this has not made progress in a while, I think we should try to implement 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2280r4.html instead 
of this approach, as I'm not sure if that's strictly conforming, and a more 
holistic approach is likely to lead to better results.
What do people think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132398

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

[PATCH] D132398: Allow constant static members to be used with 'this'

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8458
+  Info.AccessingStaticConstantDataMember);
+  if(Info.InConstantContext)
+Info.AccessingStaticConstantDataMember = true;

When not in a constant-context, what should we be doing here? Why doesn't that 
set the variable?



Comment at: clang/lib/AST/ExprConstant.cpp:8460
+Info.AccessingStaticConstantDataMember = true;
   VisitIgnoredBaseExpression(E->getBase());
   return Success(MD);

Will this visit end up looking into OTHER things here?  I guess I'm concerned 
about something like:

`this->get_some_other_type().static_func()` and us skipping the `this->` for 
THAT, despite it not being a static call in that context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132398

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

[PATCH] D132398: Allow constant static members to be used with 'this'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: aaron.ballman, clang-language-wg.
aaron.ballman added a comment.

Thanks for this! Can you add some more details to the patch description so it's 
more clear from there why the patch is necessary? Also, can you add a release 
note for the fix?

I'm pretty sure the changes here are correct, but I want to take another run at 
the standards wording to be sure.




Comment at: clang/lib/AST/ExprConstant.cpp:931
 
+/// TODO: add doc.
+bool AccessingStaticConstantDataMember = false;

Add the docs?



Comment at: clang/lib/AST/ExprConstant.cpp:8741-8743
+  if (Info.AccessingStaticConstantDataMember) {
+return Success(E);
+  }





Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:925
+}
\ No newline at end of file


You should add a new line back to the end of the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132398

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

[PATCH] D132398: Allow constant static members to be used with 'this'

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 454947.
usaxena95 added a comment.

Format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132398

Files:
  clang/lib/AST/ExprConstant.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
@@ -908,3 +908,17 @@
   return testDefaultArgForParam() + testDefaultArgForParam((E)1);
 }
 }
+
+namespace GH56246 {
+struct S {
+static consteval int hello() { return 1;}
+static constexpr int world() { return 1;}
+
+consteval int baz() const { return this->hello();}
+// Previously these two failed because of the use of `this` in the 
constant expressions.
+int foo() const { return this->hello() + this->world(); }
+constexpr int bar() const { return this->hello() + this->world();}
+} s;
+static_assert(s.bar() == 2);
+static_assert(s.baz() == 1);
+}
\ No newline at end of file
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -43,7 +43,9 @@
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CurrentSourceLocExprScope.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/OSLog.h"
 #include "clang/AST/OptionalDiagnostic.h"
 #include "clang/AST/RecordLayout.h"
@@ -926,6 +928,8 @@
 /// later on (such as a use of an undefined global).
 bool CheckingPotentialConstantExpression = false;
 
+/// TODO: add doc.
+bool AccessingStaticConstantDataMember = false;
 /// Whether we're checking for an expression that has undefined behavior.
 /// If so, we will produce warnings if we encounter an operation that is
 /// always undefined.
@@ -8449,6 +8453,10 @@
   // Handle static member functions.
   if (const CXXMethodDecl *MD = dyn_cast(E->getMemberDecl())) {
 if (MD->isStatic()) {
+  llvm::SaveAndRestore StaticMember(
+  Info.AccessingStaticConstantDataMember);
+  if(Info.InConstantContext)
+Info.AccessingStaticConstantDataMember = true;
   VisitIgnoredBaseExpression(E->getBase());
   return Success(MD);
 }
@@ -8730,6 +8738,9 @@
 if (Info.checkingPotentialConstantExpression())
   return false;
 if (!Info.CurrentCall->This) {
+  if (Info.AccessingStaticConstantDataMember) {
+return Success(E);
+  }
   if (Info.getLangOpts().CPlusPlus11)
 Info.FFDiag(E, diag::note_constexpr_this) << E->isImplicit();
   else


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -908,3 +908,17 @@
   return testDefaultArgForParam() + testDefaultArgForParam((E)1);
 }
 }
+
+namespace GH56246 {
+struct S {
+static consteval int hello() { return 1;}
+static constexpr int world() { return 1;}
+
+consteval int baz() const { return this->hello();}
+// Previously these two failed because of the use of `this` in the constant expressions.
+int foo() const { return this->hello() + this->world(); }
+constexpr int bar() const { return this->hello() + this->world();}
+} s;
+static_assert(s.bar() == 2);
+static_assert(s.baz() == 1);
+}
\ No newline at end of file
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -43,7 +43,9 @@
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CurrentSourceLocExprScope.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/OSLog.h"
 #include "clang/AST/OptionalDiagnostic.h"
 #include "clang/AST/RecordLayout.h"
@@ -926,6 +928,8 @@
 /// later on (such as a use of an undefined global).
 bool CheckingPotentialConstantExpression = false;
 
+/// TODO: add doc.
+bool AccessingStaticConstantDataMember = false;
 /// Whether we're checking for an expression that has undefined behavior.
 /// If so, we will produce warnings if we encounter an operation that is
 /// always undefined.
@@ -8449,6 +8453,10 @@
   // Handle static member functions.
   if (const CXXMethodDecl *MD = dyn_cast(E->getMemberDecl())) {
 if (MD->isStatic()) {
+  llvm::SaveAndRestore StaticMember(
+  Info.AccessingStaticConstantDataMember);
+  if(Info.InConstantContext)
+Info.AccessingStaticConstantDataMember = true;
   VisitIgnoredBaseExpression(E->getBase());
   return Success(MD);
 }
@@ -8730,6 +8738,9 @@
 if