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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D113545#3616814 <https://reviews.llvm.org/D113545#3616814>, @rsmith wrote:

> A few comments, but they're all minor things or FIXMEs. I'm happy for this to land once they're addressed.

Thanks for reviewing!



================
Comment at: clang/lib/Sema/SemaLookup.cpp:2000-2004
+  // Class and enumeration member names can be found by name lookup in any
+  // context in which a definition of the type is reachable.
+  if (auto *ECD = dyn_cast<EnumConstantDecl>(ND))
+    return getSema().hasReachableDeclaration(
+        cast<NamedDecl>(ECD->getDeclContext()));
----------------
rsmith wrote:
> ChuanqiXu wrote:
> > rsmith wrote:
> > > I don't think this is quite right. Given:
> > > 
> > > ```
> > > export module M {
> > > export enum E1 { e1 };
> > > enum E2 { e2 };
> > > export using E2U = E2;
> > > enum E3 { e3 };
> > > export E3 f();
> > > ```
> > > 
> > > ... the intent is:
> > > 
> > > ```
> > > import M;
> > > int main() {
> > >   auto a = E1::e1; // OK, namespace-scope name E1 is visible and e1 is reachable
> > >   auto b = e1; // OK, namespace-scope name e1 is visible
> > >   auto c = E2::e2; // error, namespace-scope name E2 is not visible
> > >   auto d = e2; // error, namespace-scope name e2 is not visible
> > >   auto e = E2U::e2; // OK, namespace-scope name E2U is visible and E2::e2 is reachable
> > >   auto f = E3::e3; // error, namespace-scope name E3 is not visible
> > >   auto g = e3; // error, namespace-scope name e3 is not visible
> > >   auto h = decltype(f())::e3; // OK, namespace-scope name f is visible and E3::e3 is reachable
> > > }
> > > ```
> > > 
> > > Instead of doing the check in this function, I think we need to consider the scope in which we're doing a lookup: if that scope is a class or enumeration scope, and the class or enumeration has a reachable definition, then we don't do any visibility checks for its members.
> > Oh, your example makes sense. The current implementation would accept `d` and `g` unexpectedly. I spent some time to look into this one. And I find it is not so easy to fix. I left a FIXME below and I would file an issue if this patch landed. Do you think this is OK?
> > 
> > BTW, I feel like the wording of spec need to be adjusted too. From the wording, I feel like `d` and `g` should be accepted.
> Yes, I'm fine with having FIXMEs for things that don't work yet -- so long as we don't regress things (particularly, so long as we don't regress module map module support, which some of our users are heavily relying on).
> 
> I agree that the wording here is not capturing the rules properly; I've mailed the CWG reflector and it sounds like Davis is going to fix that :)
Thanks! I believe it wouldn't be a regression.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113545/new/

https://reviews.llvm.org/D113545