[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
This revision was automatically updated to reflect the committed changes.
Closed by commit rL321781: [Modules] Allow modules specified by
-fmodule-map-file to shadow implicitly… (authored by bruno, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D31269?vs=92738=128590#toc
Repository:
rL LLVM
https://reviews.llvm.org/D31269
Files:
cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td
cfe/trunk/include/clang/Basic/Module.h
cfe/trunk/include/clang/Lex/HeaderSearch.h
cfe/trunk/include/clang/Lex/ModuleMap.h
cfe/trunk/lib/Basic/Module.cpp
cfe/trunk/lib/Lex/HeaderSearch.cpp
cfe/trunk/lib/Lex/ModuleMap.cpp
cfe/trunk/lib/Lex/PPDirectives.cpp
cfe/trunk/test/Modules/Inputs/shadow/A1/A.h
cfe/trunk/test/Modules/Inputs/shadow/A1/module.modulemap
cfe/trunk/test/Modules/Inputs/shadow/A2/A.h
cfe/trunk/test/Modules/Inputs/shadow/A2/module.modulemap
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A1/Foo.h
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A1/module.modulemap
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A1/sys/A.h
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A1/sys/A2.h
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A2/Foo.h
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A2/module.modulemap
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A2/sys/A.h
cfe/trunk/test/Modules/Inputs/shadowed-submodule/A2/sys/A2.h
cfe/trunk/test/Modules/Inputs/shadowed-submodule/Foo/module.modulemap
cfe/trunk/test/Modules/shadow.m
cfe/trunk/test/Modules/shadowed-submodule.m
Index: cfe/trunk/include/clang/Lex/ModuleMap.h
===
--- cfe/trunk/include/clang/Lex/ModuleMap.h
+++ cfe/trunk/include/clang/Lex/ModuleMap.h
@@ -195,6 +195,17 @@
/// header.
llvm::DenseMap UmbrellaDirs;
+ /// \brief The set of modules provided explicitly (e.g. by -fmodule-map-file),
+ /// which are allowed to shadow other implicitly discovered modules.
+ llvm::DenseSet ExplicitlyProvidedModules;
+
+ bool mayShadowModuleBeingParsed(Module *ExistingModule,
+ bool IsExplicitlyProvided) {
+assert(!ExistingModule->Parent && "expected top-level module");
+return !IsExplicitlyProvided &&
+ ExplicitlyProvidedModules.count(ExistingModule);
+ }
+
/// \brief The set of attributes that can be attached to a module.
struct Attributes {
/// \brief Whether this is a system module.
@@ -475,9 +486,9 @@
///
/// \returns The found or newly-created module, along with a boolean value
/// that will be true if the module is newly-created.
- std::pair findOrCreateModule(StringRef Name, Module *Parent,
- bool IsFramework,
- bool IsExplicit);
+ std::pair
+ findOrCreateModule(StringRef Name, Module *Parent, bool IsFramework,
+ bool IsExplicit, bool UsesExplicitModuleMapFile = false);
/// \brief Create a 'global module' for a C++ Modules TS module interface
/// unit.
@@ -502,6 +513,11 @@
Module *inferFrameworkModule(const DirectoryEntry *FrameworkDir,
bool IsSystem, Module *Parent);
+ /// \brief Create a new top-level module that is shadowed by
+ /// \p ShadowingModule.
+ Module *createShadowedModule(StringRef Name, bool IsFramework,
+ Module *ShadowingModule);
+
/// \brief Retrieve the module map file containing the definition of the given
/// module.
///
@@ -587,6 +603,8 @@
/// \brief Marks this header as being excluded from the given module.
void excludeHeader(Module *Mod, Module::Header Header);
+ void setExplicitlyProvided(Module *Mod);
+
/// \brief Parse the given module map file, and record any modules we
/// encounter.
///
@@ -606,10 +624,15 @@
/// \param ExternModuleLoc The location of the "extern module" declaration
///that caused us to load this module map file, if any.
///
+ /// \param IsExplicitlyProvided Whether this module map file was provided
+ /// explicitly by the user (e.g. -fmodule-map-file), rather than found
+ /// implicitly.
+ ///
/// \returns true if an error occurred, false otherwise.
bool parseModuleMapFile(const FileEntry *File, bool IsSystem,
- const DirectoryEntry *HomeDir, FileID ID = FileID(),
- unsigned *Offset = nullptr,
+ const DirectoryEntry *HomeDir,
+ bool IsExplicitlyProvided = false,
+ FileID ID = FileID(), unsigned *Offset = nullptr,
SourceLocation ExternModuleLoc = SourceLocation());
/// \brief Dump the contents of the module map, for debugging purposes.
Index: cfe/trunk/include/clang/Lex/HeaderSearch.h
===
--- cfe/trunk/include/clang/Lex/HeaderSearch.h
+++
[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
bruno added a comment. > It might make more sense to have the module loaded from the AST file shadow > the module from the module map, especially for an explicit module build, now > that we have that functionality.) +1, seems a much more consistent model. Comment at: lib/Lex/ModuleMap.cpp:2574-2575 + llvm::SaveAndRestore OldExplicit(CurrentModuleMapIsExplicitlyProvided); + CurrentModuleMapIsExplicitlyProvided |= IsExplicitlyProvided; + rsmith wrote: > It would seem cleaner to make this a member of `ModuleMapParser` (and > explicitly pass down the flag when parsing an `extern module` declaration). > Is there a reason to use (essentially) global state for this? I don't believe there's any reason for using a global state here (and Ben doesn't recall any specific reason either). I changed the patch to pass down the flag and it works fine. https://reviews.llvm.org/D31269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This makes a lot of sense to me. See also r302463: I think we probably want three levels of shadowing here: the main input shadows -fmodule-map-file, which shadows module maps loaded implicitly. (There's also a question of what we do with module map information loaded from an AST file: currently when that happens, we ignore the tokens for the parsed module map entirely. It might make more sense to have the module loaded from the AST file shadow the module from the module map, especially for an explicit module build, now that we have that functionality.) Comment at: lib/Lex/ModuleMap.cpp:2574-2575 + llvm::SaveAndRestore OldExplicit(CurrentModuleMapIsExplicitlyProvided); + CurrentModuleMapIsExplicitlyProvided |= IsExplicitlyProvided; + It would seem cleaner to make this a member of `ModuleMapParser` (and explicitly pass down the flag when parsing an `extern module` declaration). Is there a reason to use (essentially) global state for this? https://reviews.llvm.org/D31269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
bruno added a comment. Ping! https://reviews.llvm.org/D31269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
bruno added a comment. Ping! https://reviews.llvm.org/D31269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
bruno created this revision.
When modules come from module map files explicitly specified by
-fmodule-map-file= arguments, allow those to override/shadow modules
with the same name that are found implicitly by header search. If such a
module is looked up by name (e.g. @import), we will always find the one
from -fmodule-map-file. If we try to use a shadowed module by including
one of its headers report an error.
This enables developers to force use of a specific copy of their module
to be used if there are multiple copies that would otherwise be visible,
for example if they develop modules that are installed in the default
search paths.
We're using this internally for a couple years now, it would be nice to have it
upstreamed.
Patch originally by Ben Langmuir,
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20151116/143425.html
Based on cfe-dev discussion:
http://lists.llvm.org/pipermail/cfe-dev/2015-November/046164.html
https://reviews.llvm.org/D31269
Files:
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/Module.h
include/clang/Lex/HeaderSearch.h
include/clang/Lex/ModuleMap.h
lib/Basic/Module.cpp
lib/Frontend/CompilerInstance.cpp
lib/Frontend/FrontendActions.cpp
lib/Lex/HeaderSearch.cpp
lib/Lex/ModuleMap.cpp
lib/Lex/PPDirectives.cpp
test/Modules/Inputs/shadow/A1/A.h
test/Modules/Inputs/shadow/A1/module.modulemap
test/Modules/Inputs/shadow/A2/A.h
test/Modules/Inputs/shadow/A2/module.modulemap
test/Modules/shadow.m
Index: test/Modules/shadow.m
===
--- /dev/null
+++ test/Modules/shadow.m
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/shadow/A1 -I %S/Inputs/shadow/A2 %s -fsyntax-only 2>&1 | FileCheck %s -check-prefix=REDEFINITION
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/shadow/A1/module.modulemap -fmodule-map-file=%S/Inputs/shadow/A2/module.modulemap %s -fsyntax-only 2>&1 | FileCheck %s -check-prefix=REDEFINITION
+// REDEFINITION: error: redefinition of module 'A'
+// REDEFINITION: note: previously defined
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/shadow/A1/module.modulemap -I %S/Inputs/shadow %s -verify
+
+@import A1;
+@import A2;
+@import A;
+
+#import "A2/A.h" // expected-note {{implicitly imported}}
+// expected-error@A2/module.modulemap:1 {{import of shadowed module 'A'}}
+// expected-note@A1/module.modulemap:1 {{previous definition}}
+
+#if defined(A2_A_h)
+#error got the wrong definition of module A
+#elif !defined(A1_A_h)
+#error missing definition from A1
+#endif
Index: test/Modules/Inputs/shadow/A2/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A2/module.modulemap
@@ -0,0 +1,5 @@
+module A {
+ header "A.h"
+}
+
+module A2 {}
Index: test/Modules/Inputs/shadow/A2/A.h
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A2/A.h
@@ -0,0 +1 @@
+#define A2_A_h
Index: test/Modules/Inputs/shadow/A1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A1/module.modulemap
@@ -0,0 +1,5 @@
+module A {
+ header "A.h"
+}
+
+module A1 {}
Index: test/Modules/Inputs/shadow/A1/A.h
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A1/A.h
@@ -0,0 +1 @@
+#define A1_A_h
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1869,13 +1869,17 @@
if (!SuggestedModule.getModule()->isAvailable()) {
Module::Requirement Requirement;
Module::UnresolvedHeaderDirective MissingHeader;
+ Module *ShadowingModule = nullptr;
Module *M = SuggestedModule.getModule();
// Identify the cause.
(void)M->isAvailable(getLangOpts(), getTargetInfo(), Requirement,
- MissingHeader);
+ MissingHeader, ShadowingModule);
if (MissingHeader.FileNameLoc.isValid()) {
Diag(MissingHeader.FileNameLoc, diag::err_module_header_missing)
<< MissingHeader.IsUmbrella << MissingHeader.FileName;
+ } else if (ShadowingModule) {
+Diag(M->DefinitionLoc, diag::err_module_shadowed) << M->Name;
+Diag(ShadowingModule->DefinitionLoc, diag::note_previous_definition);
} else {
Diag(M->DefinitionLoc, diag::err_module_unavailable)
<< M->getFullModuleName() << Requirement.second << Requirement.first;
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -29,6 +29,7 @@
#include
