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

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

dexonsmith added a comment.

It seems like using `CachedModuleLoads` wasn't quite right in the first place. I wonder if `-fmodule-file=<pcm>` could/should use a different map (maybe one that doesn't exist yet), instead of replacing the key in CachedModuleLoads.

In D111543#3054922 <https://reviews.llvm.org/D111543#3054922>, @jansvoboda11 wrote:

> It'd be great if someone could clarify why the extra `IdentifierInfo` would trip up name resolution.

My guess would be that the attribute parsing does a lookup of "Identifier" and finds the entity introduced by `-fmodule-file` instead of the one that's part of the declaration. Not sure though.

Are you sure this is only for Objective-C interfaces? Doesn't also happen if you name a module after a C++ class or something, and reference it from an attribute?

> Suggestions on naming the test file are also welcome.

Doesn't really seem to be about visibility. Maybe `module-name-used-by-objc-bridge`?



================
Comment at: clang/include/clang/Lex/ModuleMap.h:101-102
 
   /// The top-level modules that are known.
   llvm::StringMap<Module *> Modules;
 
----------------
I wonder, could `-fmodule-file=...` be using this map instead (already a StringMap), and the map below reserved for "real" identifiers?



================
Comment at: clang/include/clang/Lex/ModuleMap.h:104-106
+  /// Module loading cache that includes submodules, indexed by module names.
   /// nullptr is stored for modules that are known to fail to load.
+  llvm::StringMap<Module *> CachedModuleLoads;
----------------
In builds with lots of fine-grained submodules, this will add many tiny allocations (one for each map entry). Maybe use `StringMap<Module*,BumpPtrAllocator>` to group them together?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111543