iains added a comment. thanks for amending the test cases they are now much easier to read and maintain.
This looks like a useful step forward, hopefully we can resolve any outstanding issue soon and get it landed. Did you try wider testing (e.g. with lldb and other parts of the toolchain?) ================ 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: > > 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? ... ``` 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
