[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

Mon Oct 31 06:27:11 PDT 2022

aaron.ballman added inline comments.

================
Comment at: clang/lib/Sema/Sema.cpp:460-462
+        Scope S(TUScope, Scope::DeclScope, getDiagnostics());
+        PushDeclContext(&S, DC);
+        PushOnScopeChains(ND, &S);
----------------
erichkeane wrote:
> mizvekov wrote:
> > aaron.ballman wrote:
> > > Is it valid to use a local variable as a `Scope` object? I would assume that scope goes away at the end of this compound statement? I didn't spot other cases where we did this -- usually we call `Parser::EnterScope()` which allocates a new `Scope` that `Sema` then uses.
> > I don't see anything problematic about allocating it on the stack like this, implementation-wise.
> > 
> > I thought it would be a bit counter-intuitive to use parser functions here, since these declarations are entirely synthetic.
> I dont know much about "Scope", as it is particularly used in Parsing, but I DO see the comment says: "A scope is a transient data structure that is used while parsing the program.  It assists with resolving identifiers to the appropriate declaration."
> 
> Which means to me it doesn't need to out-live its parsing time.  I THINK it gets replaced with a CXXScope at one point, but I dont know when/where that happens.
> 
The situation I'm worried about (and maybe it's not a valid concern) is that `Parser::EnterScope` uses a cache of scopes that this bypasses and I'm not certain whether that's a good thing or not. I *think* it might be reasonable (`Scope` is just a holder for a `DeclContext` and that's the object which has all the declarations added to and removed from, and that context outlives the `Scope` object), but it's worth double-checking.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886