* [PATCH] c++: wrong ambiguity in accessing static field [PR112744]
@ 2023-11-29 15:45 Marek Polacek
2023-11-29 17:23 ` Patrick Palka
2023-11-29 18:58 ` [PATCH] " Jason Merrill
0 siblings, 2 replies; 9+ messages in thread
From: Marek Polacek @ 2023-11-29 15:45 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
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;
+ }
+ 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." }
+}
base-commit: 3d104d93a7011146b0870ab160613147adb8d9b3
--
2.42.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 15:45 [PATCH] c++: wrong ambiguity in accessing static field [PR112744] Marek Polacek
@ 2023-11-29 17:23 ` Patrick Palka
2023-11-29 17:43 ` Marek Polacek
2023-11-29 18:58 ` [PATCH] " Jason Merrill
1 sibling, 1 reply; 9+ messages in thread
From: Patrick Palka @ 2023-11-29 17:23 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jason Merrill
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?
struct A { constexpr static int a = 0; };
struct D : private A {};
void f() {
D{}.A::a; // #1 GCC (and Clang) currently rejects
}
template<class T>
void g() {
D{}.T::a; // #2 GCC currently rejects, Clang accepts?!
}
template void g<A>();
> + }
> + 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." }
> +}
>
> base-commit: 3d104d93a7011146b0870ab160613147adb8d9b3
> --
> 2.42.0
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 17:23 ` Patrick Palka
@ 2023-11-29 17:43 ` Marek Polacek
2023-11-29 20:28 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Marek Polacek @ 2023-11-29 17:43 UTC (permalink / raw)
To: Patrick Palka; +Cc: GCC Patches, Jason Merrill
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?
> struct A { constexpr static int a = 0; };
> struct D : private A {};
>
> void f() {
> D{}.A::a; // #1 GCC (and Clang) currently rejects
> }
>
> template<class T>
> void g() {
> D{}.T::a; // #2 GCC currently rejects, Clang accepts?!
> }
>
> template void g<A>();
Thanks for looking at the patch and the testcase. I'll add it.
> > + }
> > + 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." }
> > +}
> >
> > base-commit: 3d104d93a7011146b0870ab160613147adb8d9b3
> > --
> > 2.42.0
> >
> >
>
Marek
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 15:45 [PATCH] c++: wrong ambiguity in accessing static field [PR112744] Marek Polacek
2023-11-29 17:23 ` Patrick Palka
@ 2023-11-29 18:58 ` Jason Merrill
2023-11-29 21:59 ` Marek Polacek
1 sibling, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2023-11-29 18:58 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 11/29/23 10:45, 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...
Hmm, won't using ba_any unconditionally break ambiguous base checking
for non-static data members?
> @@ -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] ()
Why a lambda?
> + {
> + if (identifier_p (name))
> + {
> + tree m = lookup_member (scope, name, /*protect=*/0,
> + /*want_type=*/false, tf_none);
> + if (!m || VAR_P (m))
Do you want shared_member_p here?
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 17:43 ` Marek Polacek
@ 2023-11-29 20:28 ` Jason Merrill
2023-11-29 22:01 ` [PATCH v2] " Marek Polacek
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2023-11-29 20:28 UTC (permalink / raw)
To: Marek Polacek, Patrick Palka; +Cc: GCC Patches
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.
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 18:58 ` [PATCH] " Jason Merrill
@ 2023-11-29 21:59 ` Marek Polacek
0 siblings, 0 replies; 9+ messages in thread
From: Marek Polacek @ 2023-11-29 21:59 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Wed, Nov 29, 2023 at 01:58:31PM -0500, Jason Merrill wrote:
> On 11/29/23 10:45, 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...
>
> Hmm, won't using ba_any unconditionally break ambiguous base checking for
> non-static data members?
Yes. I thought not but that's only because we weren't properly testing
that case (added scoped14.C, patch to follow). So that settles that.
> > @@ -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] ()
>
> Why a lambda?
Only so that I can set 'ba' once and make it const. I don't believe it
deserves a named function. It seems more readable than using a ?:.
> > + {
> > + if (identifier_p (name))
> > + {
> > + tree m = lookup_member (scope, name, /*protect=*/0,
> > + /*want_type=*/false, tf_none);
> > + if (!m || VAR_P (m))
>
> Do you want shared_member_p here?
That looks like the right thing to use, thanks.
Marek
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 20:28 ` Jason Merrill
@ 2023-11-29 22:01 ` Marek Polacek
2023-11-30 16:42 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Marek Polacek @ 2023-11-29 22:01 UTC (permalink / raw)
To: Jason Merrill; +Cc: Patrick Palka, GCC Patches
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>.
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.
+
+void f() {
+ D{}.A::a; // { dg-error "inaccessible" }
+ D{}.::A::a;
+}
+
+template<class T>
+void g() {
+ D{}.T::a;
+}
+
+template void g<A>();
base-commit: 220fe41fd4085e91a49e62dd815628ec4883a4ea
--
2.42.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-29 22:01 ` [PATCH v2] " Marek Polacek
@ 2023-11-30 16:42 ` Jason Merrill
2023-11-30 21:44 ` Marek Polacek
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2023-11-30 16:42 UTC (permalink / raw)
To: Marek Polacek; +Cc: Patrick Palka, GCC Patches
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.
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] c++: wrong ambiguity in accessing static field [PR112744]
2023-11-30 16:42 ` Jason Merrill
@ 2023-11-30 21:44 ` Marek Polacek
0 siblings, 0 replies; 9+ messages in thread
From: Marek Polacek @ 2023-11-30 21:44 UTC (permalink / raw)
To: Jason Merrill; +Cc: Patrick Palka, GCC Patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-30 21:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-29 15:45 [PATCH] c++: wrong ambiguity in accessing static field [PR112744] Marek Polacek
2023-11-29 17:23 ` Patrick Palka
2023-11-29 17:43 ` Marek Polacek
2023-11-29 20:28 ` Jason Merrill
2023-11-29 22:01 ` [PATCH v2] " Marek Polacek
2023-11-30 16:42 ` Jason Merrill
2023-11-30 21:44 ` Marek Polacek
2023-11-29 18:58 ` [PATCH] " Jason Merrill
2023-11-29 21:59 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).