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);
----------------
mizvekov wrote:
> 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.
Great, thank you for double-checking, I appreciate it!
================
Comment at: clang/test/AST/ast-dump-overloaded-operators.cpp:27
// CHECK-NEXT: | `-ParmVarDecl {{.*}} <col:18> col:19{{( imported)?}} 'E'
-// CHECK-NEXT: `-FunctionDecl {{.*}} <line:14:1, line:18:1> line:14:6{{(
imported)?}} test 'void ()'
+// CHECK-NEXT: -FunctionDecl {{.*}} <line:14:1, line:18:1> line:14:6{{(
imported)?}} test 'void ()'
// CHECK-NEXT: `-CompoundStmt {{.*}} <col:13, line:18:1>
----------------
mizvekov wrote:
> aaron.ballman wrote:
> > mizvekov wrote:
> > > aaron.ballman wrote:
> > > > mizvekov wrote:
> > > > > aaron.ballman wrote:
> > > > > > This looks like a benign typo -- we still match the line because
> > > > > > FileCheck will match partial lines, but I'm pretty sure nothing in
> > > > > > your patch would have necessitated this change. Then again, you
> > > > > > make this change in a lot of tests, so maybe I'm wrong -- in which
> > > > > > case, what changed?
> > > > > What is happening here (and in all the other such instances) is that
> > > > > on the `import` case, this declaration stops being the last one on
> > > > > the TU. So the beginning of the line would match on `|-` instead of
> > > > > ``-`, but the non-import case this remains the last one.
> > > > >
> > > > > So I simply relaxed the match.
> > > > Hmmm, I think it'd help to show what new lines are now showing up so we
> > > > can validate that they make sense in context. WDYT?
> > > It's the new lines from the synthesized `va_list_tag`. I think they would
> > > be noise on this these tests, they are testing something completely
> > > unrelated.
> > >
> > > But they do show up on the ast-json tests where we are basically dumping
> > > the whole TU.
> > Oh, so we're adding those to the *end* of the translation unit, not at the
> > beginning?
> We are adding them at the beginning, but it seems we import the other stuff
> before adding them, so on the import tests they show up on the end.
>
> The thing here is that we end up importing them, but then adding new ones
> from the current Sema anyway. But this is fine as most other import things,
> they get separate declarations but merged in any case so they have the same
> canonical decl.
>
> The way we avoid doing that for the other synthesized builtins around it, is
> that we just skip creating them if we find their `Identifier` has been
> created somehow, which is a fairly strange way to test this.
>
> This seemed even less appropriate for `va_list_tag`, since some ABIs put it
> into `std` namespace.
>
> And it seems to me that avoiding creating them again for the current Sema is
> a fairly minimal optimization, and it could make we miss diagnosing imports
> where those things are inconsistent between contexts.
>
> WDYT?
> We are adding them at the beginning, but it seems we import the other stuff
> before adding them, so on the import tests they show up on the end.
That really surprises me -- I thought we would export the AST in order and
import it in order. I'm a bit worried that out-of-order AST nodes may break
invariants.
> This seemed even less appropriate for va_list_tag, since some ABIs put it
> into std namespace.
Yeah, identifying things by name in the frontend is rarely a trivial exercise,
we try to avoid it whenever possible.
> And it seems to me that avoiding creating them again for the current Sema is
> a fairly minimal optimization, and it could make we miss diagnosing imports
> where those things are inconsistent between contexts.
Yeah, it doesn't strike me as a significant optimization to be worth worrying
about. My concern is more that we expect import order to match export order and
we're not matching up.
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
