[PATCH] D23858: Don't diagnose non-modular includes when we are not compiling a module

[Manman Ren via cfe-commits]

manmanren added a comment.

In https://reviews.llvm.org/D23858#525647, @rsmith wrote:

> It seems to me like this is papering over an issue rather than fixing it.


I guess that is why we introduced fmodule-implementation-of, to work around issues like "relative includes to a VFS-mapped module". Ben should know more about the background.

> `diagnoseHeaderInclusion` calls `isHeaderInUmbrellaDirs` here, which is presumably failing to determine that `Nonmodular/A.h` is in the umbrella directory for the `Nonmodular` module.


Bruno and I looked at this together yesterday. With vfsoverlay, the header files in the module map are using the real path, while the umbrella directories in the module map are using the virtual path. We have issue figuring out the header file from the real path is in the umbrella directory. We have longer term goals on fixing up this. But this is a regression on our side and we are hoping to get back to our previous behavior.

Cheers,
Manman


================
Comment at: lib/Lex/PPDirectives.cpp:749-750
@@ -748,3 +748,4 @@
   Module *RequestingModule = getModuleForLocation(FilenameLoc);
-  bool RequestingModuleIsModuleInterface = !SourceMgr.isInMainFile(FilenameLoc);
+  bool RequestingModuleIsModuleInterface =
+      !SourceMgr.isInMainFile(FilenameLoc) && getLangOpts().CompilingModule;
 
----------------
rsmith wrote:
> I don't see why this should depend on whether we're compiling a module. If we're asked to diagnose non-modular #includes in modular headers, why should it matter whether we entered that header textually or by triggering precompilation of the corresponding module?
I agree that here, we are actually including a non modular header from a modular header. But is it okay to not diagnose when we have specified fmodule-name and we are building the implementation of it? We should have diagnosed this when building the unit.


https://reviews.llvm.org/D23858