graydon updated this revision to Diff 82161.
graydon added a comment.
Updates to address review comments
https://reviews.llvm.org/D27852
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticLexKinds.td
include/clang/Lex/HeaderSearch.h
lib/Lex/HeaderSearch.cpp
lib/Lex/ModuleMap.cpp
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
test/Modules/implicit-private-with-different-name.m
Index: test/Modules/implicit-private-with-different-name.m
===================================================================
--- /dev/null
+++ test/Modules/implicit-private-with-different-name.m
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+
+// Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
+
+// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
+
+// Check the fixit
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level module 'APrivate' in private module map, expected a submodule of 'A'}}
+// expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"
+
+#ifndef HEADER
+#define HEADER
+#import "A/aprivate.h"
+const int *y = &APRIVATE;
+#endif
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,4 @@
+framework module APrivate {
+ header "aprivate.h"
+ export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module A {
+ header "a.h"
+ export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
@@ -0,0 +1 @@
+extern int APRIVATE;
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
@@ -0,0 +1 @@
+extern int APUBLIC;
Index: lib/Lex/ModuleMap.cpp
===================================================================
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -1490,6 +1490,42 @@
ActiveModule->NoUndeclaredIncludes = true;
ActiveModule->Directory = Directory;
+ if (!ActiveModule->Parent) {
+ StringRef MapFileName(ModuleMapFile->getName());
+ if (MapFileName.endswith("module.private.modulemap") ||
+ MapFileName.endswith("module_private.map")) {
+ // Adding a top-level module from a private modulemap is likely a
+ // user error; we check to see if there's another top-level module
+ // defined in the non-private map in the same dir, and if so emit a
+ // warning.
+ for (auto E = Map.module_begin(); E != Map.module_end(); ++E) {
+ auto const *M = E->getValue();
+ if (!M->Parent &&
+ M->Directory == ActiveModule->Directory &&
+ M->Name != ActiveModule->Name) {
+ Diags.Report(ActiveModule->DefinitionLoc,
+ diag::warn_mmap_mismatched_top_level_private)
+ << ActiveModule->Name << M->Name;
+ // The pattern we're defending against here is typically due to
+ // a module named FooPrivate which is supposed to be a submodule
+ // called Foo.Private. Emit a fixit in that case.
+ auto D =
+ Diags.Report(ActiveModule->DefinitionLoc,
+ diag::note_mmap_rename_top_level_private_as_submodule);
+ D << ActiveModule->Name << M->Name;
+ StringRef Bad(ActiveModule->Name);
+ if (Bad.consume_back("Private")) {
+ SmallString<128> Fixed = Bad;
+ Fixed.append(".Private");
+ D << FixItHint::CreateReplacement(ActiveModule->DefinitionLoc,
+ Fixed);
+ }
+ break;
+ }
+ }
+ }
+ }
+
bool Done = false;
do {
switch (Tok.Kind) {
Index: lib/Lex/HeaderSearch.cpp
===================================================================
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -194,16 +194,36 @@
Module *Module = ModMap.findModule(ModuleName);
if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps)
return Module;
-
+
+ StringRef SearchName = ModuleName;
+ Module = lookupModule(ModuleName, SearchName);
+
+ // The facility for "private modules" -- adjacent, optional module maps named
+ // module.private.modulemap that are supposed to define private submodules --
+ // is sometimes misused by frameworks that name their associated private
+ // module FooPrivate, rather than as a submodule named Foo.Private as
+ // intended. Here we compensate for such cases by looking in directories named
+ // Foo.framework, when we previously looked and failed to find a
+ // FooPrivate.framework.
+ if (!Module && SearchName.consume_back("Private"))
+ Module = lookupModule(ModuleName, SearchName);
+ return Module;
+}
+
+Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) {
+ Module *Module = nullptr;
+
// Look through the various header search paths to load any available module
// maps, searching for a module map that describes this module.
for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
if (SearchDirs[Idx].isFramework()) {
- // Search for or infer a module map for a framework.
+ // Search for or infer a module map for a framework. Here we use
+ // SearchName rather than ModuleName, to permit finding private modules
+ // named FooPrivate in buggy frameworks named Foo.
SmallString<128> FrameworkDirName;
FrameworkDirName += SearchDirs[Idx].getFrameworkDir()->getName();
- llvm::sys::path::append(FrameworkDirName, ModuleName + ".framework");
- if (const DirectoryEntry *FrameworkDir
+ llvm::sys::path::append(FrameworkDirName, SearchName + ".framework");
+ if (const DirectoryEntry *FrameworkDir
= FileMgr.getDirectory(FrameworkDirName)) {
bool IsSystem
= SearchDirs[Idx].getDirCharacteristic() != SrcMgr::C_User;
Index: include/clang/Lex/HeaderSearch.h
===================================================================
--- include/clang/Lex/HeaderSearch.h
+++ include/clang/Lex/HeaderSearch.h
@@ -547,6 +547,19 @@
void loadTopLevelSystemModules();
private:
+
+ /// \brief Lookup a module with the given module name and search-name.
+ ///
+ /// \param ModuleName The name of the module we're looking for.
+ ///
+ /// \param SearchName The "search-name" to derive filesystem paths from
+ /// when looking for the module map; this is usually equal to ModuleName,
+ /// but for compatibility with some buggy frameworks, additional attempts
+ /// may be made to find the module under a related-but-different search-name.
+ ///
+ /// \returns The module named ModuleName.
+ Module *lookupModule(StringRef ModuleName, StringRef SearchName);
+
/// \brief Retrieve a module with the given name, which may be part of the
/// given framework.
///
Index: include/clang/Basic/DiagnosticLexKinds.td
===================================================================
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -642,6 +642,12 @@
def err_mmap_expected_attribute : Error<"expected an attribute name">;
def warn_mmap_unknown_attribute : Warning<"unknown attribute '%0'">,
InGroup<IgnoredAttributes>;
+def warn_mmap_mismatched_top_level_private : Warning<
+ "top-level module '%0' in private module map, expected a submodule of '%1'">,
+ InGroup<PrivateModule>;
+def note_mmap_rename_top_level_private_as_submodule : Note<
+ "make '%0' a submodule of '%1' to ensure it can be found by name">,
+ InGroup<PrivateModule>;
def warn_auto_module_import : Warning<
"treating #%select{include|import|include_next|__include_macros}0 as an "
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -243,6 +243,7 @@
[NonModularIncludeInFrameworkModule]>;
def IncompleteModule : DiagGroup<"incomplete-module",
[IncompleteUmbrella, NonModularIncludeInModule]>;
+def PrivateModule : DiagGroup<"private-module">;
def CXX11InlineNamespace : DiagGroup<"c++11-inline-namespace">;
def InvalidNoreturn : DiagGroup<"invalid-noreturn">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
