[PATCH] D48367: [modules] Fix 37878; Autoload subdirectory modulemaps with specific LangOpts

[Yuka Takahashi via Phabricator via cfe-commits]

yamaguchi added inline comments.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:285
     // directory.
-    loadSubdirectoryModuleMaps(SearchDirs[Idx]);
+    if (ModMap.getLangOpts().ObjC1 || ModMap.getLangOpts().ObjC2)
+      loadSubdirectoryModuleMaps(SearchDirs[Idx]);
----------------
bruno wrote:
> yamaguchi wrote:
> > bruno wrote:
> > > aprantl wrote:
> > > > Are these flags also enabled in Objective-C++ mode?
> > > Looks like all this logic was introduced in r177621 to allow the names of modules to differ from the name of their subdirectory in the include path.
> > > 
> > > Instead of having this to be based on the language, it's probably better if we have it based on @import name lookup, which is the scenario where we actually currently look more aggressively, did you try that path?
> > > 
> > > This is also lacking a testcase, can you create one?
> > > Are these flags also enabled in Objective-C++ mode?
> > I think so from FrontendOptions.h:190
> > `bool isObjectiveC() const { return Lang == ObjC || Lang == ObjCXX; }`
> > 
> > > it's probably better if we have it based on @import name lookup
> > I don't think I understood what you're saying, could you explain a bit more?
> > 
> > > This is also lacking a testcase, can you create one?
> > Sure!
> > I don't think I understood what you're saying, could you explain a bit more?
> 
> This extra call to `loadSubdirectoryModuleMaps` was introduced in r177621 to allow `@import SomeName` to work even if `SomeName` doesn't match a subdirectory name. See the original commit for more details.
> 
> This means that it was never supposed to be called when the module is built via `#include` or `#import`, which is what you are getting. That said, I believe it's better if the condition for calling `loadSubdirectoryModuleMaps` checks somehow if the lookup is coming from of an `@import` instead of checking the language (we too don't want it to be called for ObjC when `#include` or `#import` are used).
> we too don't want it to be called for ObjC when #include or #import are used
https://gist.github.com/yamaguchi1024/27caba1897eb813b297a8c4785adc11d
This is one thing I could think of, it excludes `#include` but `#import` and `@import` are still treated in the same way. Do you have any idea how to do this? Is Clang differenciating the module behavior based on `#import` or `@import` in ObjC?


================
Comment at: clang/test/Modules/autoload-subdirectory.cpp:1
+// RUN: %clang -fmodules -fmodule-name=Foo -I %S/Inputs/autoload-subdirectory/ %s
+
----------------
bruno wrote:
> Instead of `%clang`, please use `%clang_cc1` and `-verify` here.
Sure


https://reviews.llvm.org/D48367