[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11403: [Modules] Add Darwin-specific compatibility module map parsing hacks
From:       Ben Langmuir <blangmuir () apple ! com>
Date:       2015-08-05 21:36:55
Message-ID: 7f46515848f76b4677f086e2ca55f933 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]