ChuanqiXu added inline comments.
================ Comment at: clang/lib/Sema/SemaLookup.cpp:1947 + // DeclModule if it isn't (transitively) imported. + if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit()) + return true; ---------------- iains wrote: > ChuanqiXu wrote: > > 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. > > OK, perhaps I am missing something - just to clarify,... > > In this (which I believe is legal like [module.unit] ex 1 TU 4. > ``` > module; > .... > module Main; > > import :ImplUnit; // this has a BMI which could contain reachable > definitions, right? > > ... > ``` > > > > Yeah, I believe this is legal according to [module.reach]p1: > A translation unit U is necessarily reachable from a point P if U is a module > interface unit on which the translation unit containing P has an interface > dependency, **or the translation unit containing P imports U**, in either > case prior to P. Since module Main imports `:ImplUnit` directly, the `:ImplUnit` TU is necessarily reachable. 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
