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).