[PATCH] D111543: [clang][modules] Stop creating `IdentifierInfo` for names of explicit modules

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

I wonder if it would be more clear going forward to name this additional cache by its purpose, so it doesn't sound more general than it really is. As currently named, it might look like all top-level modules should be stored in that map instead of the IdentifierInfo map.

Once you update the naming and/or comments to avoid that potential pitfall, this LGTM.



================
Comment at: clang/include/clang/Lex/ModuleMap.h:104-106
+  /// Top-module loading cache, indexed by module names.
+  /// nullptr is stored for modules that are known to fail to load.
+  llvm::StringMap<Module *, llvm::BumpPtrAllocator> CachedTopLevelModuleLoads;
----------------
Would be good to say why this is separate from CachedModuleLoads. Maybe, "for when an `IdentifierInfo` isn't available".

Or, should this be called `CachedExplicitModuleLoads` and be more clear about the purpose?
- Use this for explicit modules from external sources.
- Use `CachedModuleLoads` as a cache for source references where there's an `IdentifierInfo`.


================
Comment at: clang/include/clang/Lex/ModuleMap.h:714-717
+  /// Cache a top-level module load.  M might be nullptr.
+  void cacheTopLevelModuleLoad(StringRef ModuleName, Module *M) {
+    CachedTopLevelModuleLoads[ModuleName] = M;
+  }
----------------
If you change the name of the member, please change the API to match.


================
Comment at: clang/include/clang/Lex/ModuleMap.h:101-102
 
   /// The top-level modules that are known.
   llvm::StringMap<Module *> Modules;
 
----------------
dexonsmith wrote:
> I wonder, could `-fmodule-file=...` be using this map instead (already a StringMap), and the map below reserved for "real" identifiers?
> 
Can you explain in the commit message why this map can't be expanded to serve this purpose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111543