[PATCH] D11403: [Modules] Add Darwin-specific compatibility module map parsing hacks

[Ben Langmuir]

benlangmuir added inline comments.

================
Comment at: lib/Lex/ModuleMap.cpp:1891-1908
@@ +1890,20 @@
+
+  if (ActiveModule->UsesRequiresExcludedHack) {
+    // Mark this header 'textual' (see doc comment for
+    // Module::UsesRequiresExcludedHack). Although iterating over the directory
+    // is relatively expensive, in practice this only applies to the uncommonly
+    // used Tcl module on Darwin platforms.
+    std::error_code EC;
+    for (llvm::sys::fs::recursive_directory_iterator I(Dir->getName(), EC), E;
+         I != E && !EC; I.increment(EC)) {
+      if (const FileEntry *FE = SourceMgr.getFileManager().getFile(I->path())) {
+        // FIXME: Taking the name from the FileEntry is unstable and can give
+        // different results depending on how we've previously named that file
+        // in this build.
+        Module::Header Header = {FE->getName(), FE};
+        Map.addHeader(ActiveModule, Header, ModuleMap::TextualHeader);
+      }
+    }
+    return;
+  }
+
----------------
rsmith wrote:
> benlangmuir wrote:
> > rsmith wrote:
> > > Is there a better way of handling this? If the parent directory isn't itself an umbrella directory of some module, maybe you could just ignore the umbrella directory declaration for this module entirely?
> > This only affects Tcl.Private, and Tcl has an umbrella header so I don't think that will work.  The only other way I can think of making this work is to have a notion of a *directory* that is exempt from its containing umbrella, but I'm not sure that's a generally good feature and it seems like a lot more work.  Let me know if you have any suggestions though.
> Ugh, OK. In that case:
> 
>  * maybe take the file name from `I->path()` rather than `FE->getName()` (we want to imagine the files were named within the umbrella directory rather than some other way)
>  * sort the paths before you add them so that the serialized pcm doesn't depend on file system traversal order
> 
> Also, you'll be paying this file system traversal cost whenever the relevant module map is parsed, not only when the Tcl module is used -- and if it's the /usr/include module map, you'll walk this umbrella directory on every build. Just wanted to confirm you're OK with that.
> * maybe take the file name from I->path() rather than FE->getName()
> * sort the paths
Ah, good catches!

> Also, you'll be paying this file system traversal cost whenever the relevant module map is parsed,

Tcl is a framework so not affected by looking at usr/include.  But your comment made me double check where else we might parse this.  I think the answer is:

1. When Tcl is named in an import or header include.  This seems fine.
2. When we call collectAllModules()...

I'm not happy about (2) since it can happen during code completion of imports.  I suppose we're already iterating directories and parsing a tonne of other module map files here.  I'll benchmark this and see how awful it is... 


Repository:
  rL LLVM

http://reviews.llvm.org/D11403