[PATCH] Allow a header to be part of multiple modules

Daniel Jasper djasper at google.com
Mon Oct 21 01:51:22 PDT 2013


  Addressed review comments.


================
Comment at: include/clang/Lex/ModuleMap.h:213
@@ +212,3 @@
+  /// \param User Used to disambiguate if a header is present in multiple
+  /// modules. 
+  ///
----------------
Manuel Klimek wrote:
> Perhaps:
> Specifies the module the header is intended to be used from. Used to ...
Done.

================
Comment at: lib/Lex/HeaderSearch.cpp:508
@@ -505,1 +507,3 @@
 {
+  if (!HSOpts->ModuleMapFiles.empty()) {
+    for (llvm::SetVector<std::string>::iterator
----------------
Manuel Klimek wrote:
> I think an explanation of why we pre-load all modules files might help here...
Done.

================
Comment at: lib/Lex/ModuleMap.cpp:189-190
@@ +188,4 @@
+
+      // If uses need to be specified explicitly, do not report modules that
+      // are not used by 'User'.
+      if (User && LangOpts.ModulesDeclUse &&
----------------
Manuel Klimek wrote:
> How about: If uses need to be specified explicitly, we are only allowed to return modules that are explicitly used by the requesting module.
Done, thanks!

================
Comment at: lib/Lex/ModuleMap.cpp:212
@@ -188,9 +211,3 @@
     // Check again.
-    Known = Headers.find(File);
-    if (Known != Headers.end()) {
-      // If a header is not available, don't report that it maps to anything.
-      if (!Known->second.isAvailable())
-        return KnownHeader();
-
-      return Known->second;
-    }
+    if (Headers.find(File) != Headers.end())
+      return findModuleForHeader(File, User);
----------------
Manuel Klimek wrote:
> Do we need to special-case the 'exlucded' role?
Done. I wonder why this didn't need to be checked before..


http://llvm-reviews.chandlerc.com/D1951



More information about the cfe-commits mailing list