[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:       Richard Smith <richard () metafoo ! co ! uk>
Date:       2015-08-05 21:45:08
Message-ID: ca4d8301e92782128d6ce67c8b841a19 () localhost ! localdomain
[Download RAW message or body]

rsmith 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;
+  }
+
----------------
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.


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]