rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Some nits and very minor things, but I think this is generally good to go with
those fixed. Sorry for the long review delay, and thanks for working on it!
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2394
+def err_invalid_consteval_call : Error<
+ "call to consteval function '%0' could not be evaluated">;
+def err_invalid_consteval_decl_kind : Error<
----------------
could not be evaluated -> is not a constant expression
================
Comment at: clang/include/clang/Sema/Sema.h:814
+ /// Whether the AST is currently being rebuild to correct immediate
+ /// invocations. Immediate invocation candidates and references to consteval
----------------
rebuild -> rebuilt
================
Comment at: clang/include/clang/Sema/Sema.h:1078
+ /// Set of candidate for starting an immediate invocation.
+ llvm::SmallVector<ImmediateInvocationCandidate, 4>
ImmediateInvocationCandidates;
----------------
candidate -> candidates
================
Comment at: clang/include/clang/Sema/Sema.h:1081
+
+ /// Set of DeclRefExpr referencing a consteval function when used in a
+ /// context not already know to be immediately invoked.
----------------
DeclRefExpr -> DeclRefExprs
================
Comment at: clang/include/clang/Sema/Sema.h:1082
+ /// Set of DeclRefExpr referencing a consteval function when used in a
+ /// context not already know to be immediately invoked.
+ llvm::SmallPtrSet<DeclRefExpr *, 4> ReferenceToConsteval;
----------------
know -> known
================
Comment at: clang/lib/AST/ExprConstant.cpp:13618
+ if (InPlace) {
+ LValue LVal;
+ if (!::EvaluateInPlace(Result.Val, Info, LVal, this) ||
----------------
Tyker wrote:
> rsmith wrote:
> > 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.
> i believe this solution should work. without LifetimeExtendedTemporaryDecl
> because reference/pointer on temporaries are not valid results of constant
> evaluation. so the AST should never store an APValue whose LValue is a
> ConstantExpr.
OK, I think that should be fine. Please update the comment above `class
LValueExprEvaluator` to describe this additional case.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:8883
<< ConstexprKind;
+ NewFD->setConstexprKind(CSK_unspecified);
+ }
----------------
In C++20, we should probably reset to `constexpr` rather than unspecified.
Before C++20, it's better for error recovery to leave the function as
`constexpr`. (So unconditionally using `CSK_constexpr` here would be OK.)
================
Comment at: clang/lib/Sema/SemaExpr.cpp:15225
+ FD->printQualifiedName(OS, SemaRef.getASTContext().getPrintingPolicy());
+ SemaRef.Diag(CE->getBeginLoc(), diag::err_invalid_consteval_call) <<
Buffer;
+ for (auto &Note : Notes)
----------------
You can use `"%q0" to get a qualified name in a diagnostic; you don't need to
turn it into a string yourself.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63960/new/
https://reviews.llvm.org/D63960
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
