iains added inline comments.
================ Comment at: clang/lib/Sema/SemaLookup.cpp:1947 + // DeclModule if it isn't (transitively) imported. + if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit()) + return true; ---------------- ChuanqiXu wrote: > 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. (sorry for multiple iterations - I am trying to see if I missed some point ... ) ... it seems to me that in valid code `:ImplUnit` can have `Kind =` `ModulePartitionInterface` or `ModulePartitionImplementation` the second is the special case of an implementation that provides a BMI also. 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
