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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
