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: > > > 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. > > Yes, this confused me for a relative long time. It is really confusing that module partition implementation unit is importable but not module interface unit. The standard often emphasize module interface unit. From my implementation and using experience, the implementor and user could treat module partition implementation unit as an implementation partition unit if we treat all additional implementation TU as reachable. (This is what we do internally) 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
