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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:460-462
+        Scope S(TUScope, Scope::DeclScope, getDiagnostics());
+        PushDeclContext(&S, DC);
+        PushOnScopeChains(ND, &S);
----------------
aaron.ballman wrote:
> 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.
Yeah I checked that. The scope object is fairly innocuous, the `EnterScope` function uses a cache to stack them, which seems useful in performance intensive workloads, and in case the object needs to outlive the function that creates it.

But here I see no problem just making it clear that this Scope is only used here, by simply allocating it on the stack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886