mizvekov added inline comments.
================
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:
> > > > > > 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.
> We are importing in the right order.
>
> The problem is that we import the pch before we create the builtins here.
>
> Ie look at what the test is doing, it first creates a TU, then imports it
> into an empty TU `/dev/null`.
> In that last case, we end up adding the declaration for this Sema's
> `va_list_tag` after the imported stuff, but the one from the original context
> is still imported in the right order.
>
> It doesn't seem problematic to me, but would you rather we changed the import
> order around wrt the creation of the builtins?
I think changing that order is weirder in fact, it would cause part of the
current TU to come before the imported one, and then part afterwards.
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
