[PATCH] D136554: Implement CWG2631

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:9602-9604
+    return Ctx.Context ==
+               ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed ||
+           Ctx.IsCheckingDefaultArgumentOrInitializer;
----------------
Hmm, it'd be nice to not name this with the same identifier as the `bool` member on line 1333, that surprised me a little bit when I ran into it below.


================
Comment at: clang/include/clang/Sema/Sema.h:9610
+           "Must be in an expression evaluation context");
+    for (auto &Ctx : llvm::reverse(ExprEvalContexts)) {
+      if (Ctx.Context ==
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:9628
+      if (Ctx.Context == ExpressionEvaluationContext::PotentiallyEvaluated &&
+          Ctx.DelayedDefaultInitializationContext.has_value())
+        return Ctx.DelayedDefaultInitializationContext;
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:9645-9646
+      if (Ctx.Context == ExpressionEvaluationContext::PotentiallyEvaluated &&
+          !Ctx.DelayedDefaultInitializationContext.has_value() &&
+          Res.has_value())
+        break;
----------------



================
Comment at: clang/lib/AST/ExprCXX.cpp:964
+                                             DeclContext *UsedContext) {
+  size_t Size = totalSizeToAlloc<Expr *>(RewrittenExpr != nullptr ? 1 : 0);
+  auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
----------------



================
Comment at: clang/lib/AST/ExprCXX.cpp:1019
+
+  size_t Size = totalSizeToAlloc<Expr *>(RewrittenInitExpr != nullptr ? 1 : 0);
+  auto *Mem = Ctx.Allocate(Size, alignof(CXXDefaultArgExpr));
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:5927
+  bool VisitSourceLocExpr(SourceLocExpr *E) {
+    HasImmediateCalls = true;
+    return RecursiveASTVisitor<ImmediateCallVisitor>::VisitStmt(E);
----------------
It may be worth a comment that this relies on the behavior of `VisitLambdaExpr` not changing to visit the lambda body. Otherwise, nested invocations would be an issue in something like `void func(int a = []{ return std::source_location::current(); }());` because we'd claim there is an immediate call for that default arg expression when there isn't one.

As it stands, I wonder what the behavior of this code should be:
```
void func(int a =
  ({
    int val = std::souce_location::current();
    val;
  })
);
```
(Oh, wait, I know, I think we made that code invalid because GNU statement expressions in a default argument are horrifying...)

Also, it looks like this code ignores blocks while handling lambdas; shouldn't blocks be handled the same way as lambdas?

Can you also add a test for this example:
```
void func(int a =
  (int){ std::source_location::current() }
);
```
and I suppose we also should have a test for:
```
consteval int terrible_idea_dont_write_this_code_please(
  int &out,
  const int (&a)[std::source_location::current()] = { std::source_location::current() }
) { out = a[0]; return sizeof(a); }

consteval void func() {
  int a;
  static_assert(terrible_idea_dont_write_this_code_please(a) == a, "hahaha, not a chance!");
}
```
which demonstrates just how deeply bizarre WG21's decision is here -- the returned value != `out`.

And similar tests for default inits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554