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
