[PATCH] D132502: [clang][modules] Consider M affecting after mapping M.Private to M_Private

Wed Aug 24 12:49:55 PDT 2022

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

LGTM

================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:2017
     // migrate off of Foo.Private syntax.
-    if (!Sub && PP->getLangOpts().ImplicitModules && Name == "Private" &&
-        Module == Module->getTopLevelModule()) {
+    if (!Sub && Name == "Private" && Module == Module->getTopLevelModule()) {
       SmallString<128> PrivateModule(Module->Name);
----------------
jansvoboda11 wrote:
> jansvoboda11 wrote:
> > benlangmuir wrote:
> > > Not sure if this is a good idea, but have you considered having the scanner provide the module with `Foo.Private=` syntax instead? It would be nice to keep this hack contained to implicit builds if we can.
> > Interesting idea, that didn't occur to me. What implementation strategy are you thinking of?
> > 
> > If we want to avoid this hack, it seems like we'd need to pass `ModuleIdPath ImportPath` instead of `StringRef ModuleName` to `findOrCompileModuleAndReadAST()` and friends. That would allow us to look up the most specific `-fmodule-file=` argument. I'm not convinced changing multiple functions (and then changing them back when we don't need the hack) is better than extending the hack to explicit modules.
> > 
> > The upside is that we wouldn't need to report `M` as a dependency. WDYT?
> Another thing to consider: implicit build would need to propagate the information that module `Foo_Private` needs to be spelled on the command-line as `Foo.Private` to the scanner.
Okay, it sounds like the current patch is less invasive than the alternative. Thanks for talking it through.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132502