[PATCH] D158591: Add support of Windows Trace Logging macros

[Corentin Jabot via Phabricator via cfe-commits]

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