This is an archive of the discontinued LLVM Phabricator instance.

[Modules TS] Make imports from an interface unit visible to its implementation units
Needs ReviewPublic

Authored by hamzasood on Nov 24 2017, 8:48 AM.

Details

Reviewers
rsmith
boris
bruno
Summary

This provides an implementation for the changes outlined in section 4.1 of P0731r0, which clarifies the intended behaviour regarding implementation units being able to see imports made within their corresponding interface unit.

Diff Detail

Event Timeline

hamzasood created this revision.Nov 24 2017, 8:48 AM
boris edited edge metadata.Nov 24 2017, 9:37 AM

LGTM. Maybe it makes sense to also test that an unrelated translation unit that imports module 'test' sees neither 'a' nor 'b'.

Good idea, I've added that to the test.
I'll give Richard some time to look over this before committing.

hamzasood updated this revision to Diff 128284.Dec 28 2017, 3:33 AM

I've been investigating an issue regarding visibility during code synthesis, and I noticed that this patch half fixes it. I've added the rest of the fix since it's related to what's going on here, and I guess this is now a "correctly handle imports from an interface unit" patch.

I believe this fixes this bug posted on the LLVM bug tracker.

@boris if this is still okay with you after seeing the changes I've made, would you mind accepting the revision so I can commit this without Phabricator shouting at me?

boris added a comment.Dec 28 2017, 6:48 AM

I don't think it will be wise for me to accept this revision since I can't claim to have good understanding of Clang's internal modules model. I think it will be wise to have Richard take a look.

lib/Basic/Module.cpp
349

This comment (not sure about the code) sounds wrong to me: names from the interface unit before the module purview (I assume this is what "global module fragment" refers to) should not be visible in the implementation units.

hamzasood added inline comments.Dec 28 2017, 7:09 AM
lib/Basic/Module.cpp
349

Yes, the documentation comment for this function (in the header) notes that things visible to the interface unit aren’t necessarily visible to the implementation units.

There’re a few reasons why I chose for the function to count modules that’re only visible from the interface unit:

  1. The Module::Kind for a C++ module is ModuleInterfaceUnit. Since the module identifies itself as an interface unit, I figured it’d make sense for queries such as this to return what’s relevant for the interface unit.
  2. I think it’s just more useful that way. I can’t think of any situation where a module would need to know what’s visible to implementation units of other modules (and I couldn’t find any such examples in Clang). However there’re plenty of reasons why one would want to know about visibility to an interface unit (synthesising default members, instantiating templates etc.)
rsmith added inline comments.Apr 27 2018, 4:42 PM
lib/Sema/SemaDecl.cpp
16135

For completeness, you should also call getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, Loc); here. (It turns out to not actually matter right now because ModulesTS implies LocalSubmoduleVisibility, under which makeModuleVisible happens to be a no-op.)

16185–16194

This is not appropriate; generally whether we serialize to an AST file should be treated as orthogonal to whether we're in / importing a module.

The right check here is probably getLangOpts().getCompilingModule() == CMK_ModuleInterface.

hamzasood updated this revision to Diff 150649.Jun 10 2018, 4:50 AM

Addressed Richard's comments.

hamzasood marked 2 inline comments as done.Jun 10 2018, 4:53 AM
hamzasood added inline comments.
lib/Sema/SemaDecl.cpp
16185–16194

Thanks! I completely missed that lang opt.

bruno resigned from this revision.Nov 9 2020, 12:32 PM
test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp