Page MenuHomePhabricator

[Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones
ClosedPublic

Authored by bruno on Mar 22 2017, 4:39 PM.

Details

Summary

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

Diff Detail

Event Timeline

bruno created this revision.Mar 22 2017, 4:39 PM
rsmith accepted this revision.May 9 2017, 10:31 AM

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.)

lib/Lex/ModuleMap.cpp
2574–2575

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?

This revision is now accepted and ready to land.May 9 2017, 10:31 AM
bruno added a comment.Jan 3 2018, 6:17 PM

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.

lib/Lex/ModuleMap.cpp
2574–2575

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.

This revision was automatically updated to reflect the committed changes.