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

[Matheus Izvekov via Phabricator via cfe-commits]

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>
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886