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

[Ben Langmuir]

benlangmuir added inline comments.

================
Comment at: include/clang/Basic/Module.h:203-211
@@ -202,1 +202,11 @@
 
+  /// \brief Whether this module uses the 'requires excluded' hack to mark its
+  /// contents as 'textual'.
+  ///
+  /// On older Darwin SDK versions, 'requires excluded' is used to mark the
+  /// contents of the Darwin.C.excluded (assert.h) and Tcl.Private modules as
+  /// non-modular headers.  For backwards compatibility, we continue to support
+  /// this idiom for just these modules, and map the headers to 'textual' to
+  /// match the original intent.
+  unsigned UsesRequiresExcludedHack : 1;
+
----------------
rsmith wrote:
> Do we really need to write this into the `Module` object? Could we instead just track this while we're parsing the module map?
Will do.

================
Comment at: lib/Lex/ModuleMap.cpp:1590-1603
@@ +1589,16 @@
+                                 bool &IsRequiresExcludedHack) {
+  if (Feature == "excluded" || Feature == "cplusplus") {
+    std::string FullName = M->getFullModuleName();
+    if (FullName == "Darwin.C.excluded" || FullName == "Tcl.Private") {
+      // We will mark the module contents non-modular. See doc comment for
+      // Module::UsesRequiresExcludedHack.
+      IsRequiresExcludedHack = true;
+      return false;
+    } else if (FullName == "IOKit.avc") {
+      // This module was mistakenly marked 'requires cplusplus' in older Darwin
+      // SDK versions. As a backwards compatibility hack, don't add the
+      // requirement.
+      return false;
+    }
+  }
+
----------------
rsmith wrote:
> Please handle the `excluded` and `cplusplus` cases separately, to keep this special case as narrow as possible.
Will do.  Not sure why I ever combined them like that...

================
Comment at: lib/Lex/ModuleMap.cpp:1591
@@ +1590,3 @@
+  if (Feature == "excluded" || Feature == "cplusplus") {
+    std::string FullName = M->getFullModuleName();
+    if (FullName == "Darwin.C.excluded" || FullName == "Tcl.Private") {
----------------
rsmith wrote:
> Seems like overkill to compute the full module name for every module that `requires cplusplus`. Instead, how about walking the module path a component at a time and checking you get the expected sequence of components?
Makes sense, will do.

================
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:
> 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.


Repository:
  rL LLVM

http://reviews.llvm.org/D11403