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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:460-462
+        Scope S(TUScope, Scope::DeclScope, getDiagnostics());
+        PushDeclContext(&S, DC);
+        PushOnScopeChains(ND, &S);
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886