[PATCH] D136554: Implement CWG2631

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/ExprCXX.h:1315
+  // Retrieve the rewritten init expression (for an init expression containing
+  // immediate calls) With the top level FullExpr and ConstantExpr stripped off.
+  const Expr *getAdjustedRewrittenExpr() const;
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:1349-1350
+      DeclContext *Context = nullptr;
+
+      bool isValid() const { return Decl != nullptr; }
+    };
----------------
This can now be removed as it's unused.


================
Comment at: clang/lib/AST/ExprCXX.cpp:991-999
+Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() {
+  assert(hasRewrittenInit() &&
+         "expected this CXXDefaultArgExpr to have a rewritten init.");
+  Expr *Init = getRewrittenExpr();
+  if (auto *E = dyn_cast_if_present<FullExpr>(Init))
+    if (!isa<ConstantExpr>(E))
+      return E->getSubExpr();
----------------
Rather than duplicate the logic, we usually lean on the fact that we can cast away const on any AST node because they're all dynamically allocated (and thus are not const on the object's first definition).

I think the const version of this function should be written to dispatch to the non-const version (inline in the header file): `return const_cast<CXXDefaultArgExpr>(this)->getAdjustedRewrittenExpr();` -- then we avoid the risk of updating one method and forgetting to update the other.


================
Comment at: clang/lib/AST/ExprCXX.cpp:1042
+/// Get the initialization expression that will be used.
+const Expr *CXXDefaultInitExpr::getExpr() const {
+  assert(Field->getInClassInitializer() && "initializer hasn't been parsed");
----------------
Same suggestion here as above.


================
Comment at: clang/test/CodeGenCXX/default-arguments-with-immediate.cpp:2-3
+// RUN: %clang_cc1 -std=c++2a -triple x86_64-elf-gnu %s -emit-llvm -o - | FileCheck %s
+
+
+consteval int immediate() { return 0;}
----------------



================
Comment at: clang/test/CodeGenCXX/default-arguments-with-immediate.cpp:22
+
+// CHECK: define {{.*}} i32 @_ZL3extv()
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Move this check line up to where ext is declared?
> I moved it after `test_function`, which where it is defined, because it is a static function, it is only defined if odr used, and only odr used if the parameter is defaulted.
Ah, okay, that makes sense then.


================
Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:3-4
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2b %s
+
+
+consteval int undefined();  // expected-note 4 {{declared here}}
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554