This is an archive of the discontinued LLVM Phabricator instance.

[C++20] [Modules] Mark imported module as imported if not exported
ClosedPublic

Authored by ChuanqiXu on Dec 21 2021, 3:27 AM.

Details

Summary

The intention of this patch is described in the title. I met the problem in the original test. The original behavior of the test is clearly incorrect.

Then I found this patch could solve the problem. The module Templ imported in bar is not recorded as Imported. And the original behavior looks odd. It only maintains the Exports field but not Imports field. So I am not sure if the behavior was intended or it is just an oversight. I did simple test but I am not sure if this change is good.

Diff Detail

Unit TestsFailed

Event Timeline

ChuanqiXu requested review of this revision.Dec 21 2021, 3:27 AM
ChuanqiXu created this revision.
ChuanqiXu retitled this revision from [C++20] [Coroutines] Mark imported module as imported if not exported to [C++20] [Modules] Mark imported module as imported if not exported.
ChuanqiXu edited the summary of this revision. (Show Details)Dec 21 2021, 3:30 AM

as all exports are also imports, is there a reason we don;t also add the exports into the import table? is Exports really 'ExportedImports' and 'Imports' really 'NotExportedImports'? If that's the case, this patch is fine.

clang/lib/Sema/SemaModule.cpp
386

This comment will need adjusting.

as all exports are also imports, is there a reason we don;t also add the exports into the import table?

I am not sure if it has an intentional reason or it is just an oversight. From my practice, the Imports field in C++20 modules is empty all the time.

is Exports really 'ExportedImports' and 'Imports' really 'NotExportedImports'? If that's the case, this patch is fine.

Yes. From the codes, only export import X would add X to the exported list. And if we write import X, the Imports field is empty too.

ChuanqiXu updated this revision to Diff 396007.Dec 23 2021, 4:21 AM

Address comments.

urnathan accepted this revision.Dec 23 2021, 4:35 AM

lgtm!

This revision is now accepted and ready to land.Dec 23 2021, 4:35 AM
This revision was landed with ongoing or failed builds.Dec 23 2021, 4:52 AM
This revision was automatically updated to reflect the committed changes.