| Message ID | 20230920143747.1479843-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: missing SFINAE in grok_array_decl [PR111493] | expand |
On Wed, 20 Sep 2023, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk? ... and perhaps 13? > > -- >8 -- > > This fixes some missed SFINAE in grok_array_decl when checking a C++23 > multidimensional subscript operator expression. > > Note the existing pedwarn code paths are a backward compability fallback > for treating invalid a[x, y, z] as a[(x, y, z)], but this should only be > done outside of a SFINAE context I think. > > PR c++/111493 > > gcc/cp/ChangeLog: > > * decl2.cc (grok_array_decl): Guard errors with tf_error. > In the pedwarn code paths, return error_mark_node when in > a SFINAE context. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp23/subscript15.C: New test. > --- > gcc/cp/decl2.cc | 36 +++++++++++++++--------- > gcc/testsuite/g++.dg/cpp23/subscript15.C | 24 ++++++++++++++++ > 2 files changed, 47 insertions(+), 13 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp23/subscript15.C > > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index b402befba6d..6eb6d8c57d6 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -477,7 +477,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > { > /* If it would be valid albeit deprecated expression in > C++20, just pedwarn on it and treat it as if wrapped > - in (). */ > + in () unless we're in a SFINAE context. */ > + if (!(complain & tf_error)) > + return error_mark_node; > pedwarn (loc, OPT_Wcomma_subscript, > "top-level comma expression in array subscript " > "changed meaning in C++23"); > @@ -487,7 +489,7 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > = build_x_compound_expr_from_vec (orig_index_exp_list, > NULL, complain); > if (orig_index_exp == error_mark_node) > - expr = error_mark_node; > + return error_mark_node; > release_tree_vector (orig_index_exp_list); > } > } > @@ -512,22 +514,29 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > { > if ((*index_exp_list)->is_empty ()) > { > - error_at (loc, "built-in subscript operator without expression " > - "list"); > + if (complain & tf_error) > + error_at (loc, "built-in subscript operator without expression " > + "list"); > return error_mark_node; > } > tree idx = build_x_compound_expr_from_vec (*index_exp_list, NULL, > tf_none); > if (idx != error_mark_node) > - /* If it would be valid albeit deprecated expression in C++20, > - just pedwarn on it and treat it as if wrapped in (). */ > - pedwarn (loc, OPT_Wcomma_subscript, > - "top-level comma expression in array subscript " > - "changed meaning in C++23"); > + { > + /* If it would be valid albeit deprecated expression in C++20, > + just pedwarn on it and treat it as if wrapped in () unless > + we're in a SFINAE context. */ > + if (!(complain & tf_error)) > + return error_mark_node; > + pedwarn (loc, OPT_Wcomma_subscript, > + "top-level comma expression in array subscript " > + "changed meaning in C++23"); > + } > else > { > - error_at (loc, "built-in subscript operator with more than one " > - "expression in expression list"); > + if (complain & tf_error) > + error_at (loc, "built-in subscript operator with more than one " > + "expression in expression list"); > return error_mark_node; > } > index_exp = idx; > @@ -561,8 +570,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > swapped = true, array_expr = p2, index_exp = i1; > else > { > - error_at (loc, "invalid types %<%T[%T]%> for array subscript", > - type, TREE_TYPE (index_exp)); > + if (complain & tf_error) > + error_at (loc, "invalid types %<%T[%T]%> for array subscript", > + type, TREE_TYPE (index_exp)); > return error_mark_node; > } > > diff --git a/gcc/testsuite/g++.dg/cpp23/subscript15.C b/gcc/testsuite/g++.dg/cpp23/subscript15.C > new file mode 100644 > index 00000000000..1528ee71306 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp23/subscript15.C > @@ -0,0 +1,24 @@ > +// PR c++/111493 > +// { dg-do compile { target c++23 } } > + > +template<class T, class... Ts> > +concept CartesianIndexable = requires(T t, Ts... ts) { t[ts...]; }; > + > +static_assert(!CartesianIndexable<int>); > +static_assert(!CartesianIndexable<int, int>); > +static_assert(!CartesianIndexable<int, int, int>); > + > +static_assert(!CartesianIndexable<int*>); > +static_assert(CartesianIndexable<int*, int>); > +static_assert(!CartesianIndexable<int*, int, int>); > + > +template<class... Ts> > +struct A { > + void operator[](Ts...); > +}; > + > +static_assert(!CartesianIndexable<A<>, int>); > +static_assert(CartesianIndexable<A<int>, int>); > +static_assert(!CartesianIndexable<A<int>>); > +static_assert(!CartesianIndexable<A<int>, int, int>); > +static_assert(CartesianIndexable<A<int, int>, int, int>); > -- > 2.42.0.216.gbda494f404 > >
On Wed, 20 Sep 2023, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk? > > -- >8 -- > > This fixes some missed SFINAE in grok_array_decl when checking a C++23 > multidimensional subscript operator expression. > > Note the existing pedwarn code paths are a backward compability fallback > for treating invalid a[x, y, z] as a[(x, y, z)], but this should only be > done outside of a SFINAE context I think. > > PR c++/111493 > > gcc/cp/ChangeLog: > > * decl2.cc (grok_array_decl): Guard errors with tf_error. > In the pedwarn code paths, return error_mark_node when in > a SFINAE context. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp23/subscript15.C: New test. > --- > gcc/cp/decl2.cc | 36 +++++++++++++++--------- > gcc/testsuite/g++.dg/cpp23/subscript15.C | 24 ++++++++++++++++ > 2 files changed, 47 insertions(+), 13 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp23/subscript15.C > > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index b402befba6d..6eb6d8c57d6 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -477,7 +477,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > { > /* If it would be valid albeit deprecated expression in > C++20, just pedwarn on it and treat it as if wrapped > - in (). */ > + in () unless we're in a SFINAE context. */ > + if (!(complain & tf_error)) > + return error_mark_node; It occurred to me that we could check for tf_error much earlier, before we call build_x_compound_expr_from_vec and build_new_op, since they're only used here to implement the backward compatibilty fallback. Perhaps the following is better, then: -- >8 -- Subject: [PATCH] c++: missing SFINAE in grok_array_decl [PR111493] We should guard both the diagnostic and backward compatibilty fallback code with tf_error, so that in a SFINAE context we don't issue any diagnostics and correctly recognize ill-formed C++23 multidimensional subscript operator expressions. PR c++/111493 gcc/cp/ChangeLog: * decl2.cc (grok_array_decl): Guard diagnostic and backward compatibility fallback code paths with tf_error. gcc/testsuite/ChangeLog: * g++.dg/cpp23/subscript15.C: New test. --- gcc/cp/decl2.cc | 15 +++++++++++--- gcc/testsuite/g++.dg/cpp23/subscript15.C | 25 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp23/subscript15.C diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index b402befba6d..6ac27cbc15f 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -459,7 +459,10 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, { expr = build_op_subscript (loc, array_expr, index_exp_list, &overload, complain & tf_decltype); - if (expr == error_mark_node) + if (expr == error_mark_node + /* Don't do the backward compatibility fallback in a SFINAE + context. */ + && (complain & tf_error)) { tree idx = build_x_compound_expr_from_vec (*index_exp_list, NULL, tf_none); @@ -510,6 +513,11 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, if (index_exp == NULL_TREE) { + if (!(complain & tf_error)) + /* Don't do the backward compatibility fallback in a SFINAE + context. */ + return error_mark_node; + if ((*index_exp_list)->is_empty ()) { error_at (loc, "built-in subscript operator without expression " @@ -561,8 +569,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, swapped = true, array_expr = p2, index_exp = i1; else { - error_at (loc, "invalid types %<%T[%T]%> for array subscript", - type, TREE_TYPE (index_exp)); + if (complain & tf_error) + error_at (loc, "invalid types %<%T[%T]%> for array subscript", + type, TREE_TYPE (index_exp)); return error_mark_node; } diff --git a/gcc/testsuite/g++.dg/cpp23/subscript15.C b/gcc/testsuite/g++.dg/cpp23/subscript15.C new file mode 100644 index 00000000000..fece96be96b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp23/subscript15.C @@ -0,0 +1,25 @@ +// PR c++/111493 +// { dg-do compile { target c++23 } } + +template<class T, class... Ts> +concept CartesianIndexable = requires(T t, Ts... ts) { t[ts...]; }; + +static_assert(!CartesianIndexable<int>); +static_assert(!CartesianIndexable<int, int>); +static_assert(!CartesianIndexable<int, int, int>); + +static_assert(!CartesianIndexable<int*>); +static_assert(CartesianIndexable<int*, int>); +static_assert(!CartesianIndexable<int*, int, int>); +static_assert(!CartesianIndexable<int*, int*>); + +template<class... Ts> +struct A { + void operator[](Ts...); +}; + +static_assert(!CartesianIndexable<A<>, int>); +static_assert(CartesianIndexable<A<int>, int>); +static_assert(!CartesianIndexable<A<int>>); +static_assert(!CartesianIndexable<A<int>, int, int>); +static_assert(CartesianIndexable<A<int, int>, int, int>);
On 9/20/23 11:03, Patrick Palka wrote: > On Wed, 20 Sep 2023, Patrick Palka wrote: > >> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for >> trunk? >> >> -- >8 -- >> >> This fixes some missed SFINAE in grok_array_decl when checking a C++23 >> multidimensional subscript operator expression. >> >> Note the existing pedwarn code paths are a backward compability fallback >> for treating invalid a[x, y, z] as a[(x, y, z)], but this should only be >> done outside of a SFINAE context I think. >> >> PR c++/111493 >> >> gcc/cp/ChangeLog: >> >> * decl2.cc (grok_array_decl): Guard errors with tf_error. >> In the pedwarn code paths, return error_mark_node when in >> a SFINAE context. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/cpp23/subscript15.C: New test. >> --- >> gcc/cp/decl2.cc | 36 +++++++++++++++--------- >> gcc/testsuite/g++.dg/cpp23/subscript15.C | 24 ++++++++++++++++ >> 2 files changed, 47 insertions(+), 13 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/cpp23/subscript15.C >> >> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc >> index b402befba6d..6eb6d8c57d6 100644 >> --- a/gcc/cp/decl2.cc >> +++ b/gcc/cp/decl2.cc >> @@ -477,7 +477,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, >> { >> /* If it would be valid albeit deprecated expression in >> C++20, just pedwarn on it and treat it as if wrapped >> - in (). */ >> + in () unless we're in a SFINAE context. */ >> + if (!(complain & tf_error)) >> + return error_mark_node; > > It occurred to me that we could check for tf_error much earlier, before > we call build_x_compound_expr_from_vec and build_new_op, since they're > only used here to implement the backward compatibilty fallback. Perhaps > the following is better, then: This version is OK. > -- >8 -- > > Subject: [PATCH] c++: missing SFINAE in grok_array_decl [PR111493] > > We should guard both the diagnostic and backward compatibilty fallback > code with tf_error, so that in a SFINAE context we don't issue any > diagnostics and correctly recognize ill-formed C++23 multidimensional > subscript operator expressions. > > PR c++/111493 > > gcc/cp/ChangeLog: > > * decl2.cc (grok_array_decl): Guard diagnostic and backward > compatibility fallback code paths with tf_error. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp23/subscript15.C: New test. > --- > gcc/cp/decl2.cc | 15 +++++++++++--- > gcc/testsuite/g++.dg/cpp23/subscript15.C | 25 ++++++++++++++++++++++++ > 2 files changed, 37 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp23/subscript15.C > > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index b402befba6d..6ac27cbc15f 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -459,7 +459,10 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > { > expr = build_op_subscript (loc, array_expr, index_exp_list, > &overload, complain & tf_decltype); > - if (expr == error_mark_node) > + if (expr == error_mark_node > + /* Don't do the backward compatibility fallback in a SFINAE > + context. */ > + && (complain & tf_error)) > { > tree idx = build_x_compound_expr_from_vec (*index_exp_list, NULL, > tf_none); > @@ -510,6 +513,11 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > > if (index_exp == NULL_TREE) > { > + if (!(complain & tf_error)) > + /* Don't do the backward compatibility fallback in a SFINAE > + context. */ > + return error_mark_node; > + > if ((*index_exp_list)->is_empty ()) > { > error_at (loc, "built-in subscript operator without expression " > @@ -561,8 +569,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, > swapped = true, array_expr = p2, index_exp = i1; > else > { > - error_at (loc, "invalid types %<%T[%T]%> for array subscript", > - type, TREE_TYPE (index_exp)); > + if (complain & tf_error) > + error_at (loc, "invalid types %<%T[%T]%> for array subscript", > + type, TREE_TYPE (index_exp)); > return error_mark_node; > } > > diff --git a/gcc/testsuite/g++.dg/cpp23/subscript15.C b/gcc/testsuite/g++.dg/cpp23/subscript15.C > new file mode 100644 > index 00000000000..fece96be96b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp23/subscript15.C > @@ -0,0 +1,25 @@ > +// PR c++/111493 > +// { dg-do compile { target c++23 } } > + > +template<class T, class... Ts> > +concept CartesianIndexable = requires(T t, Ts... ts) { t[ts...]; }; > + > +static_assert(!CartesianIndexable<int>); > +static_assert(!CartesianIndexable<int, int>); > +static_assert(!CartesianIndexable<int, int, int>); > + > +static_assert(!CartesianIndexable<int*>); > +static_assert(CartesianIndexable<int*, int>); > +static_assert(!CartesianIndexable<int*, int, int>); > +static_assert(!CartesianIndexable<int*, int*>); > + > +template<class... Ts> > +struct A { > + void operator[](Ts...); > +}; > + > +static_assert(!CartesianIndexable<A<>, int>); > +static_assert(CartesianIndexable<A<int>, int>); > +static_assert(!CartesianIndexable<A<int>>); > +static_assert(!CartesianIndexable<A<int>, int, int>); > +static_assert(CartesianIndexable<A<int, int>, int, int>);
diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index b402befba6d..6eb6d8c57d6 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -477,7 +477,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, { /* If it would be valid albeit deprecated expression in C++20, just pedwarn on it and treat it as if wrapped - in (). */ + in () unless we're in a SFINAE context. */ + if (!(complain & tf_error)) + return error_mark_node; pedwarn (loc, OPT_Wcomma_subscript, "top-level comma expression in array subscript " "changed meaning in C++23"); @@ -487,7 +489,7 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, = build_x_compound_expr_from_vec (orig_index_exp_list, NULL, complain); if (orig_index_exp == error_mark_node) - expr = error_mark_node; + return error_mark_node; release_tree_vector (orig_index_exp_list); } } @@ -512,22 +514,29 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, { if ((*index_exp_list)->is_empty ()) { - error_at (loc, "built-in subscript operator without expression " - "list"); + if (complain & tf_error) + error_at (loc, "built-in subscript operator without expression " + "list"); return error_mark_node; } tree idx = build_x_compound_expr_from_vec (*index_exp_list, NULL, tf_none); if (idx != error_mark_node) - /* If it would be valid albeit deprecated expression in C++20, - just pedwarn on it and treat it as if wrapped in (). */ - pedwarn (loc, OPT_Wcomma_subscript, - "top-level comma expression in array subscript " - "changed meaning in C++23"); + { + /* If it would be valid albeit deprecated expression in C++20, + just pedwarn on it and treat it as if wrapped in () unless + we're in a SFINAE context. */ + if (!(complain & tf_error)) + return error_mark_node; + pedwarn (loc, OPT_Wcomma_subscript, + "top-level comma expression in array subscript " + "changed meaning in C++23"); + } else { - error_at (loc, "built-in subscript operator with more than one " - "expression in expression list"); + if (complain & tf_error) + error_at (loc, "built-in subscript operator with more than one " + "expression in expression list"); return error_mark_node; } index_exp = idx; @@ -561,8 +570,9 @@ grok_array_decl (location_t loc, tree array_expr, tree index_exp, swapped = true, array_expr = p2, index_exp = i1; else { - error_at (loc, "invalid types %<%T[%T]%> for array subscript", - type, TREE_TYPE (index_exp)); + if (complain & tf_error) + error_at (loc, "invalid types %<%T[%T]%> for array subscript", + type, TREE_TYPE (index_exp)); return error_mark_node; } diff --git a/gcc/testsuite/g++.dg/cpp23/subscript15.C b/gcc/testsuite/g++.dg/cpp23/subscript15.C new file mode 100644 index 00000000000..1528ee71306 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp23/subscript15.C @@ -0,0 +1,24 @@ +// PR c++/111493 +// { dg-do compile { target c++23 } } + +template<class T, class... Ts> +concept CartesianIndexable = requires(T t, Ts... ts) { t[ts...]; }; + +static_assert(!CartesianIndexable<int>); +static_assert(!CartesianIndexable<int, int>); +static_assert(!CartesianIndexable<int, int, int>); + +static_assert(!CartesianIndexable<int*>); +static_assert(CartesianIndexable<int*, int>); +static_assert(!CartesianIndexable<int*, int, int>); + +template<class... Ts> +struct A { + void operator[](Ts...); +}; + +static_assert(!CartesianIndexable<A<>, int>); +static_assert(CartesianIndexable<A<int>, int>); +static_assert(!CartesianIndexable<A<int>>); +static_assert(!CartesianIndexable<A<int>, int, int>); +static_assert(CartesianIndexable<A<int, int>, int, int>);