[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:21:01
Message-ID: 022343bade91cb8128d6c36d8c17c4cd () localhost ! localdomain
[Download RAW message or body]
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
_______________________________________________
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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic