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

[Richard Smith]

rsmith 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;
+
----------------
Do we really need to write this into the `Module` object? Could we instead just track this while we're parsing the module map?

================
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;
+    }
+  }
+
----------------
Please handle the `excluded` and `cplusplus` cases separately, to keep this special case as narrow as possible.

================
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") {
----------------
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?

================
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;
+  }
+
----------------
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?


Repository:
  rL LLVM

http://reviews.llvm.org/D11403