aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
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:
> 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.
> It doesn't seem problematic to me, but would you rather we changed the import
> order around wrt the creation of the builtins?
Ahhh, I understand now what's going on there. I don't think we need to change
the order now that I understand why the behavior is what it is (I was
forgetting that we import the PCH and then create builtins).
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
