On Thu, Nov 30, 2023 at 11:42:36AM -0500, Jason Merrill wrote: > On 11/29/23 17:01, Marek Polacek wrote: > > On Wed, Nov 29, 2023 at 03:28:44PM -0500, Jason Merrill wrote: > > > On 11/29/23 12:43, Marek Polacek wrote: > > > > On Wed, Nov 29, 2023 at 12:23:46PM -0500, Patrick Palka wrote: > > > > > On Wed, 29 Nov 2023, Marek Polacek wrote: > > > > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > > > > > Now that I'm posting this patch, I think you'll probably want me to > > > > > > use > > > > > > ba_any unconditionally. That works too; g++.dg/tc1/dr52.C just > > > > > > needs > > > > > > a trivial testsuite tweak: > > > > > > 'C' is not an accessible base of 'X' > > > > > > v. > > > > > > 'C' is an inaccessible base of 'X' > > > > > > We should probably unify those messages... > > > > > > > > > > > > -- >8 -- > > > > > > Given > > > > > > > > > > > > struct A { constexpr static int a = 0; }; > > > > > > struct B : A {}; > > > > > > struct C : A {}; > > > > > > struct D : B, C {}; > > > > > > > > > > > > we give the "'A' is an ambiguous base of 'D'" error for > > > > > > > > > > > > D{}.A::a; > > > > > > > > > > > > which seems wrong: 'a' is a static data member so there is only one > > > > > > copy > > > > > > so it can be unambiguously referred to even if there are multiple A > > > > > > objects. clang++/MSVC/icx agree. > > > > > > > > > > > > PR c++/112744 > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > * typeck.cc (finish_class_member_access_expr): When accessing > > > > > > a static data member, use ba_any for lookup_base. > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > * g++.dg/lookup/scoped11.C: New test. > > > > > > * g++.dg/lookup/scoped12.C: New test. > > > > > > * g++.dg/lookup/scoped13.C: New test. > > > > > > --- > > > > > > gcc/cp/typeck.cc | 21 ++++++++++++++++++--- > > > > > > gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++ > > > > > > gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++ > > > > > > gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++ > > > > > > 4 files changed, 60 insertions(+), 3 deletions(-) > > > > > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C > > > > > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C > > > > > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C > > > > > > > > > > > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > > > > > index e995fb6ddd7..c4de8bb2616 100644 > > > > > > --- a/gcc/cp/typeck.cc > > > > > > +++ b/gcc/cp/typeck.cc > > > > > > @@ -3476,7 +3476,7 @@ finish_class_member_access_expr (cp_expr > > > > > > object, tree name, bool template_p, > > > > > > name, scope); > > > > > > return error_mark_node; > > > > > > } > > > > > > - > > > > > > + > > > > > > if (TREE_SIDE_EFFECTS (object)) > > > > > > val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, > > > > > > val); > > > > > > return val; > > > > > > @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr > > > > > > object, tree name, bool template_p, > > > > > > return error_mark_node; > > > > > > } > > > > > > + /* NAME may refer to a static data member, in which case > > > > > > there is > > > > > > + one copy of the data member that is shared by all the > > > > > > objects of > > > > > > + the class. So NAME can be unambiguously referred to even > > > > > > if > > > > > > + there are multiple indirect base classes containing NAME. > > > > > > */ > > > > > > + const base_access ba = [scope, name] () > > > > > > + { > > > > > > + if (identifier_p (name)) > > > > > > + { > > > > > > + tree m = lookup_member (scope, name, /*protect=*/0, > > > > > > + /*want_type=*/false, tf_none); > > > > > > + if (!m || VAR_P (m)) > > > > > > + return ba_any; > > > > > > > > > > I wonder if we want to return ba_check_bit instead of ba_any so that > > > > > we > > > > > still check access of the selected base? > > > > > > > > That would certainly make sense to me. I didn't do that because > > > > I'd not seen ba_check_bit being used except as part of ba_check, > > > > but that may not mean much. > > > > > > > > So either I can tweak the lambda to return ba_check_bit rather > > > > than ba_any or use ba_check_bit unconditionally. Any opinions on that? > > > > > > The relevant passage seems to be > > > https://eel.is/c++draft/class.access.base#6 > > > after DR 52, which seems to have clarified that the pointer conversion > > > only > > > applies to non-static members. > > > > > > > > struct A { constexpr static int a = 0; }; > > > > > struct D : private A {}; > > > > > > > > > > void f() { > > > > > D{}.A::a; // #1 GCC (and Clang) currently rejects > > > > > } > > > > > > I see that MSVC also rejects it, while EDG accepts. > > > > > > https://eel.is/c++draft/class.access.base#5.1 seems to say that a is > > > accessible when named in A. > > > > > > https://eel.is/c++draft/expr.ref#7 also only constrains references to > > > non-static members. > > > > > > But first we need to look up A in D, and A's injected-class-name looked up > > > as a member of D is not accessible; it's private, and f() is not a friend, > > > and we correctly complain about that. > > > > > > If we avoid the lookup of A in D with > > > > > > D{}.::A::a; > > > > > > clang accepts it, which is consistent with accepting the template version, > > > and seems correct. > > > > > > So, I think ba_any is what we want here. > > > > Wow, that is not intuitive (to me at least). So I had it right but > > only by accident. > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > -- >8 -- > > Given > > > > struct A { constexpr static int a = 0; }; > > struct B : A {}; > > struct C : A {}; > > struct D : B, C {}; > > > > we give the "'A' is an ambiguous base of 'D'" error for > > > > D{}.A::a; > > > > which seems wrong: 'a' is a static data member so there is only one copy > > so it can be unambiguously referred to even if there are multiple A > > objects. clang++/MSVC/icx agree. > > > > The rationale for using ba_any is explained at > > <https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638676.html>. > > I'd prefer not to cite the mailing list for rationales. > > To summarize: > [class.access.base] requires conversion to a unique base subobject for > non-static data members, but it does not require that the base be unique or > accessible for static data members. > > > PR c++/112744 > > > > gcc/cp/ChangeLog: > > > > * typeck.cc (finish_class_member_access_expr): When accessing > > a static data member, use ba_any for lookup_base. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/lookup/scoped11.C: New test. > > * g++.dg/lookup/scoped12.C: New test. > > * g++.dg/lookup/scoped13.C: New test. > > * g++.dg/lookup/scoped14.C: New test. > > * g++.dg/lookup/scoped15.C: New test. > > --- > > gcc/cp/typeck.cc | 21 ++++++++++++++++++--- > > gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/lookup/scoped14.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/lookup/scoped15.C | 20 ++++++++++++++++++++ > > 6 files changed, 94 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped14.C > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped15.C > > > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > index 0839d0a4167..bf8ffaa7e75 100644 > > --- a/gcc/cp/typeck.cc > > +++ b/gcc/cp/typeck.cc > > @@ -3467,7 +3467,7 @@ finish_class_member_access_expr (cp_expr object, tree > > name, bool template_p, > > name, scope); > > return error_mark_node; > > } > > - > > + > > if (TREE_SIDE_EFFECTS (object)) > > val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val); > > return val; > > @@ -3484,9 +3484,24 @@ finish_class_member_access_expr (cp_expr object, > > tree name, bool template_p, > > return error_mark_node; > > } > > + /* NAME may refer to a static data member, in which case there is > > + one copy of the data member that is shared by all the objects of > > + the class. So NAME can be unambiguously referred to even if > > + there are multiple indirect base classes containing NAME. */ > > + const base_access ba = [scope, name] () > > + { > > + if (identifier_p (name)) > + { > > + tree m = lookup_member (scope, name, /*protect=*/0, > > + /*want_type=*/false, tf_none); > > + if (!m || shared_member_p (m)) > > + return ba_any; > > + } > > + return ba_check; > > + } (); > > + > > /* Find the base of OBJECT_TYPE corresponding to SCOPE. */ > > - access_path = lookup_base (object_type, scope, ba_check, > > - NULL, complain); > > + access_path = lookup_base (object_type, scope, ba, NULL, complain); > > if (access_path == error_mark_node) > > return error_mark_node; > > if (!access_path) > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped11.C > > b/gcc/testsuite/g++.dg/lookup/scoped11.C > > new file mode 100644 > > index 00000000000..be743522fce > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped11.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile } > > + > > +struct A { const static int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.a; > > + (void) d.A::a; > > +} > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped12.C > > b/gcc/testsuite/g++.dg/lookup/scoped12.C > > new file mode 100644 > > index 00000000000..ffa145598fd > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped12.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile } > > + > > +class A { const static int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.a; // { dg-error "private" } > > + (void) d.A::a; // { dg-error "private" } > > +} > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped13.C > > b/gcc/testsuite/g++.dg/lookup/scoped13.C > > new file mode 100644 > > index 00000000000..970e1aa833e > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped13.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile } > > + > > +struct A { const static int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.x; // { dg-error ".struct D. has no member named .x." } > > + (void) d.A::x; // { dg-error ".struct A. has no member named .x." } > > +} > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped14.C > > b/gcc/testsuite/g++.dg/lookup/scoped14.C > > new file mode 100644 > > index 00000000000..141aa0d2b1a > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped14.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile { target c++11 } } > > + > > +struct A { int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.a; // { dg-error "request for member .a. is ambiguous" } > > + (void) d.A::a; // { dg-error ".A. is an ambiguous base of .D." } > > +} > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped15.C > > b/gcc/testsuite/g++.dg/lookup/scoped15.C > > new file mode 100644 > > index 00000000000..d450a41a617 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped15.C > > @@ -0,0 +1,20 @@ > > +// PR c++/112744 > > +// { dg-do compile { target c++11 } } > > + > > +struct A { constexpr static int a = 0; }; > > +struct D : private A {}; > > + > > +// See > > <https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638676.html> > > +// for rationale. > > The injected-class-name of A is private when named in D, but if A is named > some other way, there is no requirement in [class.access.base] for static > data members that it be an accessible base. > > OK with those comment adjustments.
Thanks; pushed with that adjusted. I'm not planning to backport the patch. Marek