ChuanqiXu marked 2 inline comments as done. ChuanqiXu added a comment. In D113545#3560940 <https://reviews.llvm.org/D113545#3560940>, @iains wrote:
> the test-suite changes could be made easier to read by using the file-split > stuff we've been using for recent changes (I guess this is an older patch)? Done, this is pretty old. BTW, I've made an implementation internally and it is able to compile https://github.com/alibaba/async_simple/tree/CXX20Modules and this patch is main part of that. I want to say this one is tested more or less. --- (This section might not be related to this revision) Boris tried to compile it by GCC: https://github.com/boris-kolpackov/async_simple/tree/CXX20Modules but GCC fall in ICE You could use this one as a (not complete) test suite. ================ Comment at: clang/include/clang/AST/DeclBase.h:234-236 /// lookups that occur within that module. + /// The discarded declarations in global module fragment belongs + /// to this group too. ---------------- iains wrote: > that is not going to be sufficient to exclude them - see D126694, we > introduce a new category "ModuleUnreachable". Yeah, I am not going to edit this to avoid unnecessary rebasing work. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:1947 + // DeclModule if it isn't (transitively) imported. + if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit()) + return true; ---------------- iains wrote: > I think isModuleInterfaceUnit needs to include implementation partition > units, since they also have a BMI (is `isInterfaceOrPartition()` the right > thing to use here? I think we couldn't use `isInterfaceOrPartition()` here. The call for `isModuleInterfaceUnit()` here is sufficient. For example, we could find the example at [[ https://eel.is/c++draft/module.reach#example-1 | [module.reach]example 1 ]], here in Translation unit #5:, the definition of struct B is not reachable since the definition lives in an implementation unit. (We could make it valid by making all additional TU as reachable) Also the module interface unit shouldn't include implementation partition unit according to the wording: [[ https://eel.is/c++draft/module.unit#2 | [module.unit]p2 ]]. I agree that it is confusing since implementation partition unit is importable. But this is not our fault. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113545/new/ https://reviews.llvm.org/D113545 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
