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

[Iain Sandoe via Phabricator via cfe-commits]

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