Page MenuHomePhabricator

[C++20] [Modules] Don't create multiple global module fragment
ClosedPublic

Authored by ChuanqiXu on Dec 12 2021, 10:17 PM.

Details

Summary

Since the serialization code would recognize modules by names and the name of all global module fragment is <global>, so that the serialization code would complain for the same module.

This patch fixes this by using a global module fragment cache in Sema. Before this patch, the compiler would fail on an assertion complaining the duplicated modules.

Test Plan: check-all, an internal module library

Diff Detail

Event Timeline

ChuanqiXu requested review of this revision.Dec 12 2021, 10:17 PM
ChuanqiXu created this revision.

Seems good to me, @iains you've been looking at this?

tbaeder added inline comments.Jan 25 2022, 5:55 AM
clang/include/clang/Sema/Sema.h
2222

Typos in "gloabl" and "tranlation" in the comment. Any reason to call this a "cache"? This was confusing to me at first.

ChuanqiXu updated this revision to Diff 403130.Jan 25 2022, 9:21 PM

Address comments

Seems good to me, @iains you've been looking at this?

According to the discussion in https://github.com/llvm/llvm-project/issues/51682, it looks like Iains plans to implement something like P1184R2 (I found that is your paper!) in clang. So I added him before.

(Might you make the status as green?)

clang/include/clang/Sema/Sema.h
2222

This is consistent with the current style in Sema. You could find many example by searching Cache; in Sema.h. I think the semantic is "Cache something to avoid search/create again".

Thanks, this looks functionally good to me. I'm happy for this to land with the Sema member renamed.

clang/lib/Sema/SemaModule.cpp
727

If this is supposed to be a cache, we should look for an existing global module fragment here rather than always creating a new one. Either this member shouldn't be called ...Cache or we should do a search at this point. (I think we do want to support the existence of multiple different global module fragments in a single module, for example if we have full information about multiple different module interface units for the same module loaded, so I think just renaming the member is probably the best way to go).

ChuanqiXu updated this revision to Diff 406315.Feb 6 2022, 7:39 PM

Address comments.

Although this wasn't accepted formally, both @urnathan and @rsmith says this looks good to them. So I guess it might not be bad to land this.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 7 2022, 7:53 PM
This revision was automatically updated to reflect the committed changes.

the landed version is good, with Richard's suggested member name change. Can't see a way of post-commit accepting?

the landed version is good, with Richard's suggested member name change. Can't see a way of post-commit accepting?

Yeah, maybe we can't do post-commit accept formally.