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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
