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

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

rsmith added a comment.

I expect that we get this wrong in the same way for all the `SemaDeclRefs` declarations (`StdNamespace`, `StdBadAlloc`, and `StdAlignValT`).

I think the place where this is going wrong is `ASTDeclReader::findExisting`, which is assuming that the prior declaration for normal `NamedDecl`s can be found by name lookup; that's not true for these particular implicitly-declared entities. Perhaps the simplest fix would be to add a check around here:

  } else if (DeclContext *MergeDC = getPrimaryContextForMerging(Reader, DC)) {
    DeclContext::lookup_result R = MergeDC->noload_lookup(Name);
    for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) {
      if (NamedDecl *Existing = getDeclForMerging(*I, TypedefNameForLinkage))
        if (isSameEntity(Existing, D))
          return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
                                    TypedefNameForLinkage);
    }
    // [HERE]
  }

... to see if `D` is one of our three special entities, and if so to ask `Sema` for its current declaration of that entity (without triggering deserialization) and if not, set `D` as the declaration of that entity.



================
Comment at: lib/Sema/SemaDeclCXX.cpp:8915-8919
+      if (II->isStr("std") && PrevNS->isStdNamespace() &&
+          PrevNS != getStdNamespace()) {
+        PrevNS = getStdNamespace();
+        IsStd = true;
+      }
----------------
This doesn't seem right to me. This change appears to affect the case where we already have two distinct `std` namespaces and are declaring a third, which is too late -- things already went wrong if we reach that situation.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:9215
+    if (getLangOpts().Modules)
+      getStdNamespace()->setHasExternalVisibleStorage(true);
   }
----------------
This also doesn't look right: the "has external visible storage" flag should only be set by the external source, to indicate that it has lookup results for the decl context.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920