[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D58920#1503872 <https://reviews.llvm.org/D58920#1503872>, @modocache wrote:

> Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a little while to figure out why, even using the `LazyDeclPtr` directly, I was still triggering deserialization. It turns out `dump()` causes deserialization too -- whoops!)
>
> > You should also change `FindExistingResult::~FindExistingResult` to update `Sema::StdNamespace` to point to your newly-deserialized namespace if you didn't find a prior declaration of it, so that `Sema::getStdNamespace()` finds the deserialized namespace.
>
> I haven't done this yet. I'm trying to think of a test case that would fail if this were not done properly -- or would there not be one?


I think the failure case is a little complex to set up. I think you need:

- two modules each of which contains an invisible (implicitly-declared) `std` namespace
- arrange for `Sema::StdNamespace` to point to the ID of one of them
- trigger the import of the other one
- perform a name lookup for `std` to erase the placeholder `std` lookup result from the translation unit's lookup table
- trigger the import of the first `std` (eg, by triggering the declaration of `operator new`)

At that point, there is nowhere for the newly-imported `std` namespace to find the old one: it's not in the translation unit's name lookup table, and it's not in `Sema::StdNamespace`, so we would presumably end up with two `std` namespaces not connected to one another.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920