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

[Andrew Gallagher via Phabricator via cfe-commits]

andrewjcg added a comment.

Sorry for the delay.  Just catching up on the code this covers, so apologies if the questions don't make sense.



================
Comment at: lib/Sema/SemaDeclCXX.cpp:9214-9215
     getStdNamespace()->setImplicit(true);
+    if (getLangOpts().Modules)
+      getStdNamespace()->setHasExternalVisibleStorage(true);
   }
----------------
I think you mentioned in another forum that one side effect of this change is that it's causing multiple candidates for names in `std`.  Is this the implicit align constructros/destructors?  Is this because we're marking the implicit namespace as being externally visible?


================
Comment at: lib/Serialization/ASTReader.cpp:9291-9295
     // Load pending declaration chains.
     for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
       loadPendingDeclChain(PendingDeclChains[I].first,
                            PendingDeclChains[I].second);
     PendingDeclChains.clear();
----------------
How does modules normally "connect up" definitions of the same namspace occurring in the imported module?  Is it done here (e.g. so that a name lookup will search all namespace definitions)?  Is an alternate solution to this diff to make sure this handles the `std` implicit special case?  Apologies for the naivety here -- still getting up to speed on this code.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920