[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

[Chuanqi Xu via Phabricator via cfe-commits]

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