cor3ntin added a subscriber: tbaeder. cor3ntin added a comment. Looks mostly good to me, thanks!
In D158591#4613868 <https://reviews.llvm.org/D158591#4613868>, @RIscRIpt wrote: > The current implementation of `getPredefinedExprDeclContext` could be easily > moved to a static function inside `clang/lib/Sema/SemaExpr.cpp` as it depends > only on `Sema::CurContext` WDYT? With the new name, that would make sense. we could keep close to where it is used. ================ Comment at: clang/include/clang/Sema/Sema.h:3617-3620 + /// Returns the current DeclContext that can be used to determine the value + /// for PredefinedExpr. This can be either a block, lambda, captured + /// statement, function, otherwise a nullptr. + DeclContext *getPredefinedExprDeclContext(); ---------------- ================ Comment at: clang/lib/AST/ExprConstant.cpp:7527-7529 + bool VisitPredefinedExpr(const PredefinedExpr *E) { + return StmtVisitorTy::Visit(E->getFunctionName()); + } ---------------- I wonder if we should modify the new interpreter at the same time to avoid giving more work to @tbaeder. You can model it on `ByteCodeExprGen<Emitter>::VisitSubstNonTypeTemplateParmExpr` ================ Comment at: clang/lib/Sema/Sema.cpp:1495-1499 + DeclContext *DC = CurContext; + while (DC && !isa<BlockDecl>(DC) && !isa<CapturedDecl>(DC) && + !isa<FunctionDecl>(DC) && !isa<ObjCMethodDecl>(DC)) + DC = DC->getParent(); + return dyn_cast_or_null<Decl>(DC); ---------------- RIscRIpt wrote: > RIscRIpt wrote: > > cor3ntin wrote: > > > RIscRIpt wrote: > > > > cor3ntin wrote: > > > > > I think this is reimplementing `getCurFunctionOrMethodDecl` > > > > > maybe we want to do > > > > > > > > > > ``` > > > > > if(Decl* DC = getCurFunctionOrMethodDecl()) > > > > > return DC; > > > > > return dyn_cast_or_null<Decl>(CurrentContext); > > > > > ``` > > > > > > > > > > Or something like that > > > > Well, unfortunately, not really. > > > > > > > > The previous implementation did call `getCurFunctionOrMethodDecl()`, > > > > but it returned nullptr when we were inside a RecordDecl. > > > > If you examine the implementation of `getFunctionLevelDeclContext(bool > > > > AllowLambda)`, it halts if `isa<RecordDecl>(DC)`. I tried patching > > > > `getFunctionLevelDeclContext()` to skip RecordDecl, but this triggered > > > > around 70 clang/tests. I didn't want to delve into understanding the > > > > failure of each test. If you believe there's an issue with our current > > > > code, I can allocate time to investigate each specific test case. > > > You are right, i missed that. > > > I wish we had a better name for this function but I can't think of > > > anything > > A perfect name would be `getFunctionLevelDeclContext`, but it's taken. > > Welp... I'll try to dig-into related functions, and update when I find > > something. An alternative solution would be to parameterize (in some way) > > `getFunctionLevelDeclContext` (e.g. add bool flags, or list of skippable > > types, etc.). > > A perfect name would be getFunctionLevelDeclContext, but it's taken. > No, it's not. I found a better one, which was hiding in a plain sight. > > Regarding using a common implementation: with a new function name it's clear > that we cannot do that - they serve different purposes. For example, this > function may return `BlockDecl` or `CapturedDecl` (which is accepted by > `PredefinedExpr::ComputeName`) whereas `getFunctionLevelDeclContext` cannot. Yeah, until we find another use case for this thing, this looks reasonable. I also thought of `getAnyFunctionContext` or something along those lines but.. meh. what you have is fine, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158591/new/ https://reviews.llvm.org/D158591 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
