From: Jason Merrill <jason@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: NTTP object wrapper substitution fixes [PR103346, ...]
Date: Mon, 19 Dec 2022 12:17:33 -0500 [thread overview]
Message-ID: <a810aca7-badd-afae-95b6-c748c411edaf@redhat.com> (raw)
In-Reply-To: <98f22062-9f4e-0406-cddc-cfd49a63c3e8@idea>
On 12/6/22 13:35, Patrick Palka wrote:
> This patch fixes some issues with substitution into a C++20 template
> parameter object wrapper:
>
> * The first testcase demonstrates a situation where the same_type_p
> assert in relevant case of tsubst_copy doesn't hold, because (partial)
> substitution of {int,} into the VIEW_CONVERT_EXPR wrapper yields
> A<int> but substitution into the underlying TEMPLATE_PARM_INDEX is a
> nop and yields A<T> due to tsubst's level == 1 early exit test.
We exit in that case because continuing would reduce the level to an
impossible 0. Why doesn't the preceding code find a binding for T?
> So
> this patch just gets rid of the assert; the type mismatch doesn't
> seem to be a problem in practice, I suppose because the coercion is
> from one dependent type to another.
>
> * In the second testcase, dependent substitution into the underlying
> TEMPLATE_PARM_INDEX yields a CALL_EXPR with empty TREE_TYPE, which
> tsubst_copy doesn't expect. This patch fixes this by handling empty
> TREE_TYPE the same way as a non-const TREE_TYPE. Moreover, after
> this substitution we're left with a VIEW_CONVERT_EXPR wrapping a
> CALL_EXPR instead of a TEMPLATE_PARM_INDEX, which during the subsequent
> non-dependent substitution tsubst_copy doesn't expect either. So
> this patch also relaxes tsubst_copy to accept such VIEW_CONVERT_EXPR
> too.
>
> * In the third testcase, we end up never resolving the call to
> f.modify() since tsubst_copy doesn't do overload resolution.
> This patch fixes this by moving the handling of these
> VIEW_CONVERT_EXPR wrappers from tsubst_copy to tsubst_copy_and_build.
> For good measure tsubst_copy_and_build should also handle
> REF_PARENTHESIZED_P wrappers instead of delegating to tsubst_copy.
>
> After this patch, VIEW_CONVERT_EXPR substitution is ultimately just
> moved from tsubst_copy to tsubst_copy_and_build and made more
> permissive.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
>
> PR c++/103346
> PR c++/104278
> PR c++/102553
>
> gcc/cp/ChangeLog:
>
> * pt.cc (tsubst_copy) <case VIEW_CONVERT_EXPR>: In the handling
> of C++20 template parameter object wrappers: Remove same_type_p
> assert. Accept non-TEMPLATE_PARM_INDEX inner operand. Handle
> empty TREE_TYPE on substituted inner operand. Move it to ...
> (tsubst_copy_and_build): ... here. Also handle REF_PARENTHESIZED_P
> VIEW_CONVERT_EXPRs.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp2a/nontype-class52a.C: New test.
> * g++.dg/cpp2a/nontype-class53.C: New test.
> * g++.dg/cpp2a/nontype-class54.C: New test.
> * g++.dg/cpp2a/nontype-class55.C: New test.
> ---
> gcc/cp/pt.cc | 73 ++++++++++---------
> gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C | 15 ++++
> gcc/testsuite/g++.dg/cpp2a/nontype-class53.C | 25 +++++++
> gcc/testsuite/g++.dg/cpp2a/nontype-class54.C | 23 ++++++
> gcc/testsuite/g++.dg/cpp2a/nontype-class55.C | 15 ++++
> 5 files changed, 116 insertions(+), 35 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
>
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 2d8e4fdd4b5..0a196f069ad 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -17271,42 +17271,16 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
> return maybe_wrap_with_location (op0, EXPR_LOCATION (t));
> }
> tree op = TREE_OPERAND (t, 0);
> - if (code == VIEW_CONVERT_EXPR
> - && TREE_CODE (op) == TEMPLATE_PARM_INDEX)
> - {
> - /* Wrapper to make a C++20 template parameter object const. */
> - op = tsubst_copy (op, args, complain, in_decl);
> - if (!CP_TYPE_CONST_P (TREE_TYPE (op)))
> - {
> - /* The template argument is not const, presumably because
> - it is still dependent, and so not the const template parm
> - object. */
> - tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
> - gcc_checking_assert (same_type_ignoring_top_level_qualifiers_p
> - (type, TREE_TYPE (op)));
> - if (TREE_CODE (op) == CONSTRUCTOR
> - || TREE_CODE (op) == IMPLICIT_CONV_EXPR)
> - {
> - /* Don't add a wrapper to these. */
> - op = copy_node (op);
> - TREE_TYPE (op) = type;
> - }
> - else
> - /* Do add a wrapper otherwise (in particular, if op is
> - another TEMPLATE_PARM_INDEX). */
> - op = build1 (code, type, op);
> - }
> - return op;
> - }
> /* force_paren_expr can also create a VIEW_CONVERT_EXPR. */
> - else if (code == VIEW_CONVERT_EXPR && REF_PARENTHESIZED_P (t))
> + if (code == VIEW_CONVERT_EXPR && REF_PARENTHESIZED_P (t))
> {
> op = tsubst_copy (op, args, complain, in_decl);
> op = build1 (code, TREE_TYPE (op), op);
> REF_PARENTHESIZED_P (op) = true;
> return op;
> }
> - /* We shouldn't see any other uses of these in templates. */
> + /* We shouldn't see any other uses of these in templates
> + (tsubst_copy_and_build handles C++20 tparm object wrappers). */
> gcc_unreachable ();
> }
>
> @@ -21569,12 +21543,41 @@ tsubst_copy_and_build (tree t,
>
> case NON_LVALUE_EXPR:
> case VIEW_CONVERT_EXPR:
> - if (location_wrapper_p (t))
> - /* We need to do this here as well as in tsubst_copy so we get the
> - other tsubst_copy_and_build semantics for a PARM_DECL operand. */
> - RETURN (maybe_wrap_with_location (RECUR (TREE_OPERAND (t, 0)),
> - EXPR_LOCATION (t)));
> - /* fallthrough. */
> + {
> + tree op = RECUR (TREE_OPERAND (t, 0));
> + if (location_wrapper_p (t))
> + /* We need to do this here as well as in tsubst_copy so we get the
> + other tsubst_copy_and_build semantics for a PARM_DECL operand. */
> + RETURN (maybe_wrap_with_location (op, EXPR_LOCATION (t)));
> +
> + gcc_checking_assert (TREE_CODE (t) == VIEW_CONVERT_EXPR);
> + if (REF_PARENTHESIZED_P (t))
> + /* force_paren_expr can also create a VIEW_CONVERT_EXPR. */
> + RETURN (finish_parenthesized_expr (op));
> +
> + /* Otherwise, we're dealing with a wrapper to make a C++20 template
> + parameter object const. */
> + if (TREE_TYPE (op) == NULL_TREE
> + || !CP_TYPE_CONST_P (TREE_TYPE (op)))
> + {
> + /* The template argument is not const, presumably because
> + it is still dependent, and so not the const template parm
> + object. */
> + tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
> + if (TREE_CODE (op) == CONSTRUCTOR
> + || TREE_CODE (op) == IMPLICIT_CONV_EXPR)
> + {
> + /* Don't add a wrapper to these. */
> + op = copy_node (op);
> + TREE_TYPE (op) = type;
> + }
> + else
> + /* Do add a wrapper otherwise (in particular, if op is
> + another TEMPLATE_PARM_INDEX). */
> + op = build1 (VIEW_CONVERT_EXPR, type, op);
> + }
> + RETURN (op);
> + }
>
> default:
> /* Handle Objective-C++ constructs, if appropriate. */
> diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C b/gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> new file mode 100644
> index 00000000000..ae5d5df70ac
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> @@ -0,0 +1,15 @@
> +// A version of nontype-class52.C where explicit template arguments are
> +// given in the call to f (which during deduction need to be partially
> +// substituted into the NTTP object V in f's signature).
> +// { dg-do compile { target c++20 } }
> +
> +template<class> struct A { };
> +
> +template<auto> struct B { };
> +
> +template<class T, A<T> V> void f(B<V>);
> +
> +int main() {
> + constexpr A<int> a;
> + f<int>(B<a>{});
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class53.C b/gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> new file mode 100644
> index 00000000000..9a6398c5f57
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> @@ -0,0 +1,25 @@
> +// PR c++/103346
> +// { dg-do compile { target c++20 } }
> +
> +struct Item {};
> +
> +template<class T, T... ts>
> +struct Sequence { };
> +
> +template<Item... items>
> +using ItemSequence = Sequence<Item, items...>;
> +
> +template<Item... items>
> +constexpr auto f() {
> + constexpr auto l = [](Item item) { return item; };
> + return ItemSequence<l(items)...>{};
> +}
> +
> +using ty0 = decltype(f<>());
> +using ty0 = ItemSequence<>;
> +
> +using ty1 = decltype(f<{}>());
> +using ty1 = ItemSequence<{}>;
> +
> +using ty3 = decltype(f<{}, {}, {}>());
> +using ty3 = ItemSequence<{}, {}, {}>;
> diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class54.C b/gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> new file mode 100644
> index 00000000000..8127b1f5426
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> @@ -0,0 +1,23 @@
> +// PR c++/104278
> +// { dg-do compile { target c++20 } }
> +
> +struct foo {
> + int value;
> + constexpr foo modify() const { return { value + 1 }; }
> +};
> +
> +template<foo f, bool Enable = f.value & 1>
> +struct bar {
> + static void run() { }
> +};
> +
> +template<foo f>
> +struct qux {
> + static void run() {
> + bar<f.modify()>::run();
> + }
> +};
> +
> +int main() {
> + qux<foo{}>::run();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class55.C b/gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
> new file mode 100644
> index 00000000000..afcb3d64495
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
> @@ -0,0 +1,15 @@
> +// PR c++/102553
> +// { dg-do compile { target c++20 } }
> +
> +struct s1{};
> +template<int> inline constexpr s1 ch{};
> +
> +template<s1 f> struct s2{};
> +template<s1 f> using alias1 = s2<f>;
> +
> +template<class T>
> +void general(int n) {
> + alias1<ch<1>>{};
> +}
> +
> +template void general<int>(int);
next prev parent reply other threads:[~2022-12-19 17:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-06 17:57 Patrick Palka
2022-12-06 18:35 ` Patrick Palka
2022-12-19 16:01 ` Patrick Palka
2022-12-19 17:17 ` Jason Merrill [this message]
2022-12-19 18:13 ` Patrick Palka
2022-12-19 19:56 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a810aca7-badd-afae-95b6-c748c411edaf@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=ppalka@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).