aaron.ballman added a comment.
Generally looks good, but I did have some questions.
================
Comment at: clang/lib/AST/ExprConstant.cpp:2166-2173
if (auto *FD = dyn_cast_or_null<FunctionDecl>(BaseVD)) {
- if (FD->isConsteval()) {
+ if (FD->isImmediateFunction()) {
Info.FFDiag(Loc, diag::note_consteval_address_accessible)
<< !Type->isAnyPointerType();
Info.Note(FD->getLocation(), diag::note_declared_at);
return false;
}
----------------
We can be fancy and reduce a level of indentation, NFC.
================
Comment at: clang/lib/AST/TextNodeDumper.cpp:286-287
OS << " consteval";
+ else if (FD->isImmediateFunction())
+ OS << " immediate";
if (FD->isMultiVersion())
----------------
We should probably also update JSONNodeDumper.cpp similarly so they're somewhat
consistent.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4234
+ assert(!cast<FunctionDecl>(GD.getDecl())->isImmediateFunction() &&
"consteval function should never be emitted");
// If there was no specific requested type, just convert it now.
----------------
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6335-6337
if (auto *FD = dyn_cast<FunctionDecl>(D))
- if (FD->isConsteval())
+ if (FD->isImmediateFunction())
return;
----------------
Might as well get fancy here too.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2498-2503
+ bool TraverseDecl(Decl *) { return true; }
+
+ bool TraverseType(QualType T) { return true; }
+
+ bool VisitBlockExpr(BlockExpr *T) { return true; }
+
----------------
Are these for performance reasons, so we don't traverse into them further? If
so, a comment would be helpful explaining that.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:17980-17987
+ if (auto *Call = dyn_cast<CallExpr>(E->IgnoreImplicit());
+ Call && Call->getCallee()) {
+ if (auto *DeclRef =
+ dyn_cast<DeclRefExpr>(Call->getCallee()->IgnoreImplicit()))
+ DeclRef->setIsImmediateEscalating(true);
+ } else if (auto *DeclRef = dyn_cast<DeclRefExpr>(E->IgnoreImplicit())) {
+ DeclRef->setIsImmediateEscalating(true);
----------------
Should we be asserting the given expression is either a `CallExpr` (with a
valid callee) or a `DeclRefExpr`? Otherwise, if called with something else,
we'll claim the function found an immediately escalating expression but we
won't know *what* caused that.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5022
+ SourceLocation Loc) {
+ assert(FD->isImmediateEscalating());
+
----------------
I think I'm a bit surprised to see this assert in a function named `CheckIf` --
I would assume that we'd return false in this case?
================
Comment at: clang/lib/Serialization/ASTWriterStmt.cpp:629
+ !E->refersToEnclosingVariableOrCapture() && !E->isNonOdrUse() &&
+ !E->isImmediateEscalating()) {
AbbrevToUse = Writer.getDeclRefExprAbbrev();
----------------
Can you explain why this checks that the expression is not immediate
escalating? (What test case exercises this?)
================
Comment at: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp:20
consteval X g() { return {0}; }
-void f() { g(); }
+void f() { (void)g(); }
----------------
Why is the cast to `void` added?
================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:8
+
+namespace examples {
+
----------------
Fznamznon wrote:
> cor3ntin wrote:
> > Fznamznon wrote:
> > > These examples exactly match the ones provided by P2564R3, should they be
> > > in a separate test in `CXX` directory then?
> > I don't have a string preference, should we move the paper examples? the
> > whole file?
> I meant the paper examples. I don't have a strong preference too, so in case
> it doesn't matter where the test actually is, please ignore this comment.
Because it's voted in as a DR, we should have a test in `clang/test/CXX/drs/`
with the appropriate DR number markings (and then regenerate the DR status page
as well).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151094/new/
https://reviews.llvm.org/D151094
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
