This is an archive of the discontinued LLVM Phabricator instance.

Omit sumbodule semantics for TS modules
ClosedPublic

Authored by boris on Jul 20 2017, 5:07 AM.

Details

Summary

If a TS module name has more than one component (e.g., foo.bar) then we erroneously activate the submodule semantics when encountering a module declaration in the module implementation unit (e.g., module foo.bar;).

Diff Detail

Repository
rL LLVM

Event Timeline

boris created this revision.Jul 20 2017, 5:07 AM
rsmith edited edge metadata.Aug 3 2017, 12:40 PM

This will need a test case.

lib/Frontend/CompilerInstance.cpp
1598–1612 ↗(On Diff #107486)

The strategy we've been using so far is this: components after the first in a module path name submodules (visibility groups within a module). Modules TS modules use only the first name component, which may contain periods if the module name contains periods.

So the burden should be on the code that produces a ModuleIdPath to do this flattening, not on consumers of the ModuleIdPath.

(FWIW, I think we may want to consider this later, so that import foo.bar; could import either the Modules TS module foo.bar, or the submodule bar of the module map module foo, but that will take some fundamental restructuring of how we model Module objects and their hierarchy.)

boris updated this revision to Diff 110875.Aug 13 2017, 7:38 AM

Richard, sorry for the last ping, somehow I missed your review.

I've uploaded a new revision with a test case (note that the issue is with 'module a.b' not 'import a.b' but I have tested both for good measure).

My understanding of your comment is that the rest is ok for now (since there will probably be a redesign in this area). If, however, I misunderstood and you sill want to move the id flattening to the caller, let me know.

rsmith accepted this revision.Aug 15 2017, 5:10 PM

My understanding of your comment is that the rest is ok for now (since there will probably be a redesign in this area). If, however, I misunderstood and you sill want to move the id flattening to the caller, let me know.

I'd still like the id flattening moved to the caller. I don't know when or if the redesign will ever happen, so we should aim for the state of the codebase to be consistent in the interim (which might be forever). I'm fine with that being done as a separate change after this one, though, if you'd prefer.

Do you need someone to commit this for you?

This revision is now accepted and ready to land.Aug 15 2017, 5:10 PM
boris added a comment.Aug 16 2017, 8:25 AM

I'd still like the id flattening moved to the caller. [...] I'm fine with that being done as a separate change after this one, though, if you'd prefer.

Yes, that would probably be easier.

Do you need someone to commit this for you?

Yes, please, I don't have commit rights. Though if I were given them, I could do that myself (and it would also help with the other patch, I guess).

This revision was automatically updated to reflect the committed changes.