ChuanqiXu added a comment.
@rsmith Thanks for your valuable input!
================
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:
> 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.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3754
+ // reachable definition in the set of associated entities,
+ if (AssociatedClasses.count(RD) && hasReachableDeclaration(D)) {
Visible = true;
----------------
rsmith wrote:
> We should look for a reachable definition here, not a reachable declaration.
Yeah, from the wording it should be reachable definition. But it would cause
many failures if I write hasReachableDefinition/hasVisibleDefinition here. So
it looks like a legacy defect. I chose to follow the original style here.
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
