[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

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

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:1033-1035
+  bool IsCausedByImmediateInvocation() const {
+    return ConstantExprBits.IsCausedByImmediateInvocation;
+  }
----------------
I'd remove the "CausedBy" here -- the `ConstantExpr` is our representation of the immediate invocation.

Also, please start this function name with a lowercase `i` for local consistency.


================
Comment at: clang/include/clang/Sema/Sema.h:1027
 
+  using IICandidate = llvm::PointerIntPair<ConstantExpr *, 1>;
+
----------------
`II` doesn't mean anything to a reader here; please expand the abbreviation.


================
Comment at: clang/include/clang/Sema/Sema.h:5438-5439
 
+  /// Wrap the expression in a ConstantExpr if it is a potention immediate
+  /// invocation
+  ExprResult CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl);
----------------
potention -> potential
Missing period at end of sentence.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2012-2013
 
+  if (auto *VD = LVal.getLValueBase().dyn_cast<const ValueDecl *>())
+    if (auto *FD = dyn_cast<FunctionDecl>(VD))
+      if (FD->isConsteval()) {
----------------
Please add braces to these `if` statements (their controlled statement is too long to omit the braces). As a general rule: if an inner construct needs braces, put braces on the outer constructs too.


================
Comment at: clang/lib/AST/ExprConstant.cpp:13618
+  if (InPlace) {
+    LValue LVal;
+    if (!::EvaluateInPlace(Result.Val, Info, LVal, this) ||
----------------
This isn't sufficient: the evaluation process can refer back to the object under construction (eg, via `this`), and we need an lvalue that actually names the result in order for this to work.

I think you'll need to do something like creating a suitable object (perhaps a `LifetimeExtendedTemporaryDecl`) in the caller to represent the result of the immediate invocation, and passing that into here.


================
Comment at: clang/lib/Sema/Sema.cpp:164
   isConstantEvaluatedOverride = false;
+  RebuildingImmediateInvocation = false;
 
----------------
Consider using a default member initializer instead.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8835-8836
+      // specifier.
+      if (ConstexprKind == CSK_consteval)
+        if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD))
+          if (MD->getOverloadedOperator() == OO_New ||
----------------
Please add braces here.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8836
+      if (ConstexprKind == CSK_consteval)
+        if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD))
+          if (MD->getOverloadedOperator() == OO_New ||
----------------
This rule also applies to non-member allocator functions. (Don't check for a `CXXMethodDecl` here.)


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11188-11189
 
+  ConstexprSpecKind ConstexprKind = DetermineSpecialMemberConstexprKind(
+      Constexpr, ClassDecl->hasDefaultedConstevalDefaultCtor());
+
----------------
Tyker wrote:
> rsmith wrote:
> > I don't think this is necessary: implicit special members are never `consteval`. (We use `DeclareImplicitDefaultConstructor` when there is no user-declared default constructor, in which case there can't possibly be a defaulted consteval default constructor.) I don't think you need any of the `DetermineSpecialMemberConstexprKind` machinery, nor the tracking of `DefaultedSpecialMemberIsConsteval` in `CXXRecordDecl`.
> all the code you mention is to fixe an issue i saw while working on consteval.
> the AST of
> ```
> struct A {
>     consteval A() = default;
>     consteval A(int) { }
> };
> ```
> is
> ```
> TranslationUnitDecl
> `-CXXRecordDecl <line:1:1, line:4:1> line:1:8 struct A definition
>   |-DefinitionData pass_in_registers empty standard_layout trivially_copyable trivial literal has_user_declared_ctor has_constexpr_non_copy_move_ctor can_const_default_init
>   | |-DefaultConstructor exists trivial constexpr defaulted_is_constexpr
>   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
>   | |-MoveConstructor exists simple trivial needs_implicit
>   | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
>   | |-MoveAssignment exists simple trivial needs_implicit
>   | `-Destructor simple irrelevant trivial needs_implicit
>   |-CXXRecordDecl <col:1, col:8> col:8 implicit referenced struct A
>   |-CXXConstructorDecl <line:2:5, col:27> col:15 constexpr A 'void ()' default trivial noexcept-unevaluated 0x55585ae25160
>   `-CXXConstructorDecl <line:3:5, col:24> col:15 consteval A 'void (int)'
>     |-ParmVarDecl <col:17> col:20 'int'
>     `-CompoundStmt <col:22, col:24>
> ```
> [[ https://godbolt.org/z/oRx0Ss | godbolt ]]
> notice that `A::A()` is marked as constexpr instead of consteval.
> and `A::A(int)` is marked correctly.
> 
> this is because consteval defaulted special members are not marked as consteval.
> those code changes are intended to fix that. with this patch both constructor are marked as consteval
> 
This change is incorrect: an implicit special member is never `consteval`. (Note that implicit special members are the ones the compiler declares for you, not the ones you get from `= default`.) We don't need this, and as a consequence we don't need `DefaultedSpecialMemberIsConsteval` on `DefinitionData`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15192
+      FD = cast<FunctionDecl>(Call->getCalleeDecl());
+    else if (auto *Call = dyn_cast<CXXMemberCallExpr>(InnerExpr))
+      FD = Call->getMethodDecl();
----------------
This is unreachable; a `CXXMemberCallExpr` is a `CallExpr` so this was already handled above. Can you just delete this case?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15197
+    else
+      llvm_unreachable("unhandeld decl kind");
+    assert(FD->isConsteval());
----------------
Typo unhandeld -> unhandled


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15248
+    bool ReplacingOriginal() { return true; }
+  } Transfromer(SemaRef, Rec.ReferenceToConsteval,
+                Rec.ImmediateInvocationCandidates, It);
----------------
Transfromer -> Transformer


================
Comment at: clang/lib/Sema/TreeTransform.h:3451-3452
 
-  if (auto *FE = dyn_cast<FullExpr>(Init))
-    Init = FE->getSubExpr();
-
----------------
I'm surprised that we can remove this; I'd expect this to be necessary to properly handle transforming cases such as

```
struct X { X(int); ~X(); int n; };
struct Y { explicit Y(int); };
template<typename T> void f() {
  Y y(X(sizeof(sizeof(T))).n);
}
template void f<int>();
```

... where we need to strip off an `ExprWithCleanups` to find the syntactic initializer inside it.

If you want to special-case this from `ComplexRemove`, to make sure you see all `ConstantExpr`s, can you override `TransformInitializer` there instead of changing it globally?


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

https://reviews.llvm.org/D63960