From: Jason Merrill <jason@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: detecting copy-init context during CTAD [PR102137]
Date: Tue, 8 Mar 2022 17:26:15 -0500 [thread overview]
Message-ID: <a673d270-3817-997f-9307-c5e35c11491b@redhat.com> (raw)
In-Reply-To: <d6bc7be2-8065-ee56-f13b-f855fec9b420@idea>
On 3/8/22 17:17, Patrick Palka wrote:
> On Tue, 8 Mar 2022, Jason Merrill wrote:
>
>> On 3/8/22 14:38, Patrick Palka wrote:
>>> On Tue, 8 Mar 2022, Jason Merrill wrote:
>>>
>>>> On 3/8/22 11:36, Patrick Palka wrote:
>>>>> On Mon, 7 Mar 2022, Jason Merrill wrote:
>>>>>
>>>>>> On 3/7/22 10:47, Patrick Palka wrote:
>>>>>>> On Fri, 4 Mar 2022, Jason Merrill wrote:
>>>>>>>
>>>>>>>> On 3/4/22 14:24, Patrick Palka wrote:
>>>>>>>>> Here we're failing to communicate to cp_finish_decl from
>>>>>>>>> tsubst_expr
>>>>>>>>> that we're in a copy-initialization context (via the
>>>>>>>>> LOOKUP_ONLYCONVERTING
>>>>>>>>> flag), which causes do_class_deduction to always consider
>>>>>>>>> explicit
>>>>>>>>> deduction guides when performing CTAD for a templated variable
>>>>>>>>> initializer.
>>>>>>>>>
>>>>>>>>> We could fix this by passing LOOKUP_ONLYCONVERTING appropriately
>>>>>>>>> when
>>>>>>>>> calling cp_finish_decl from tsubst_expr, but it seems
>>>>>>>>> do_class_deduction
>>>>>>>>> can determine if we're in a copy-init context by simply
>>>>>>>>> inspecting
>>>>>>>>> the
>>>>>>>>> initializer, and thus render its flags parameter unnecessary,
>>>>>>>>> which
>>>>>>>>> is
>>>>>>>>> what this patch implements. (If we were to fix this in
>>>>>>>>> tsubst_expr
>>>>>>>>> instead, I think we'd have to inspect the initializer in the
>>>>>>>>> same
>>>>>>>>> way
>>>>>>>>> in order to detect a copy-init context?)
>>>>>>>>
>>>>>>>> Hmm, does this affect conversions as well?
>>>>>>>>
>>>>>>>> Looks like it does:
>>>>>>>>
>>>>>>>> struct A
>>>>>>>> {
>>>>>>>> explicit operator int();
>>>>>>>> };
>>>>>>>>
>>>>>>>> template <class T> void f()
>>>>>>>> {
>>>>>>>> T t = A();
>>>>>>>> }
>>>>>>>>
>>>>>>>> int main()
>>>>>>>> {
>>>>>>>> f<int>(); // wrongly accepted
>>>>>>>> }
>>>>>>>>
>>>>>>>> The reverse, initializing via an explicit constructor, is caught
>>>>>>>> by
>>>>>>>> code
>>>>>>>> in
>>>>>>>> build_aggr_init much like the code your patch adds to
>>>>>>>> do_auto_deduction;
>>>>>>>> perhaps we should move/copy that code to cp_finish_decl?
>>>>>>>
>>>>>>> Ah, makes sense. Moving that code from build_aggr_init to
>>>>>>> cp_finish_decl broke things, but using it in both spots seems to
>>>>>>> work
>>>>>>> well. And I suppose we might as well use it in do_class_deduction
>>>>>>> too,
>>>>>>> since doing so lets us remove the flags parameter.
>>>>>>
>>>>>> Before removing the flags parameter please try asserting that it now
>>>>>> matches
>>>>>> is_copy_initialization and see if anything breaks.
>>>>>
>>>>> I added to do_class_deduction:
>>>>>
>>>>> gcc_assert (bool(flags & LOOKUP_ONLYCONVERTING) ==
>>>>> is_copy_initialization
>>>>> (init));
>>>>>
>>>>> Turns out removing the flags parameter breaks CTAD for new-expressions
>>>>> of the form 'new TT(x)' because in this case build_new passes just 'x'
>>>>> as the initializer to do_auto_deduction (as opposed to a single
>>>>> TREE_LIST),
>>>>> for which is_copy_initialization returns true even though it's really
>>>>> direct initalization.
>>>>>
>>>>> Also turns out we're similarly not passing the right LOOKUP_* flags to
>>>>> cp_finish_decl from instantiate_body, which breaks consideration of
>>>>> explicit conversions/deduction guides when instantiating the initializer
>>>>> of a static data member. I added some xfailed testcases for these
>>>>> situations.
>>>>
>>>> Maybe we want to check is_copy_initialization in cp_finish_decl?
>>>
>>> That seems to work nicely :) All xfailed tests for the static data
>>> member initialization case now also pass.
>>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>>> trunk?
>>>
>>> -- >8 --
>>>
>>> Subject: [PATCH] c++: detecting copy-init context during CTAD [PR102137]
>>>
>>> Here we're failing to communicate to cp_finish_decl from tsubst_expr
>>> that we're in a copy-initialization context (via the LOOKUP_ONLYCONVERTING
>>> flag), which causes us to always consider explicit deduction guides when
>>> performing CTAD for a templated variable initializer.
>>>
>>> It turns out this bug also affects consideration of explicit conversion
>>> operators for the same reason. But consideration of explicit constructors
>>> seems to do the right thing thanks to code in build_aggr_init that sets
>>> LOOKUP_ONLYCONVERTING when the initializer represents copy-initialization.
>>>
>>> This patch fixes this by making cp_finish_decl set LOOKUP_ONLYCONVERTING
>>> by inspecting the initializer like build_aggr_init does, so that callers
>>> don't need to explicitly pass this flag.
>>>
>>> PR c++/102137
>>> PR c++/87820
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> * cp-tree.h (is_copy_initialization): Declare.
>>> * decl.cc (cp_finish_decl): Set LOOKUP_ONLYCONVERTING
>>> when is_copy_initialization is true.
>>> * init.cc (build_aggr_init): Split out copy-initialization
>>> check into ...
>>> (is_copy_initialization): ... here.
>>> * pt.cc (instantiate_decl): Pass 0 instead of
>>> LOOKUP_ONLYCONVERTING as flags to cp_finish_decl.
>>
>> Any reason not to use LOOKUP_NORMAL, both here and in tsubst_expr? OK either
>> way.
>
> Thanks a lot. I'm not really sure what the consequences of using
> LOOKUP_NORMAL (which implies LOOKUP_PROTECT) instead of 0 would be here,
> and there's a bunch of other callers of cp_finish_decl which pass 0 or
> some other value that excludes LOOKUP_PROTECT. So I figure such a change
> would be better off as a separate patch that changes/audits all such
> cp_finish_decl at once.
Agreed. Not important, just curious.
>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * g++.dg/cpp0x/explicit15.C: New test.
>>> * g++.dg/cpp1z/class-deduction108.C: New test.
>>> ---
>>> gcc/cp/cp-tree.h | 1 +
>>> gcc/cp/decl.cc | 3 +
>>> gcc/cp/init.cc | 20 +++--
>>> gcc/cp/pt.cc | 3 +-
>>> gcc/testsuite/g++.dg/cpp0x/explicit15.C | 83 +++++++++++++++++++
>>> .../g++.dg/cpp1z/class-deduction108.C | 78 +++++++++++++++++
>>> 6 files changed, 181 insertions(+), 7 deletions(-)
>>> create mode 100644 gcc/testsuite/g++.dg/cpp0x/explicit15.C
>>> create mode 100644 gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
>>>
>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>>> index ac723901098..fd76909ca75 100644
>>> --- a/gcc/cp/cp-tree.h
>>> +++ b/gcc/cp/cp-tree.h
>>> @@ -7039,6 +7039,7 @@ extern void emit_mem_initializers
>>> (tree);
>>> extern tree build_aggr_init (tree, tree, int,
>>> tsubst_flags_t);
>>> extern int is_class_type (tree, int);
>>> +extern bool is_copy_initialization (tree);
>>> extern tree build_zero_init (tree, tree, bool);
>>> extern tree build_value_init (tree,
>>> tsubst_flags_t);
>>> extern tree build_value_init_noctor (tree, tsubst_flags_t);
>>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
>>> index 199ac768d43..5452f7d9cb3 100644
>>> --- a/gcc/cp/decl.cc
>>> +++ b/gcc/cp/decl.cc
>>> @@ -7968,6 +7968,9 @@ cp_finish_decl (tree decl, tree init, bool
>>> init_const_expr_p,
>>> if (type == error_mark_node)
>>> return;
>>> + if (VAR_P (decl) && is_copy_initialization (init))
>>> + flags |= LOOKUP_ONLYCONVERTING;
>>> +
>>> /* Warn about register storage specifiers except when in GNU global
>>> or local register variable extension. */
>>> if (VAR_P (decl) && DECL_REGISTER (decl) && asmspec_tree == NULL_TREE)
>>> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
>>> index 545d904c0f9..d5faa0dc519 100644
>>> --- a/gcc/cp/init.cc
>>> +++ b/gcc/cp/init.cc
>>> @@ -2019,11 +2019,7 @@ build_aggr_init (tree exp, tree init, int flags,
>>> tsubst_flags_t complain)
>>> return stmt_expr;
>>> }
>>> - if (init && init != void_type_node
>>> - && TREE_CODE (init) != TREE_LIST
>>> - && !(TREE_CODE (init) == TARGET_EXPR
>>> - && TARGET_EXPR_DIRECT_INIT_P (init))
>>> - && !DIRECT_LIST_INIT_P (init))
>>> + if (is_copy_initialization (init))
>>> flags |= LOOKUP_ONLYCONVERTING;
>>> is_global = begin_init_stmts (&stmt_expr, &compound_stmt);
>>> @@ -2331,6 +2327,20 @@ is_class_type (tree type, int or_else)
>>> return 1;
>>> }
>>> +/* Returns true iff the variable initializer INIT represents
>>> + copy-initialization (and therefore we must set LOOKUP_ONLYCONVERTING
>>> + when processing it). */
>>> +
>>> +bool
>>> +is_copy_initialization (tree init)
>>> +{
>>> + return (init && init != void_type_node
>>> + && TREE_CODE (init) != TREE_LIST
>>> + && !(TREE_CODE (init) == TARGET_EXPR
>>> + && TARGET_EXPR_DIRECT_INIT_P (init))
>>> + && !DIRECT_LIST_INIT_P (init));
>>> +}
>>> +
>>> /* Build a reference to a member of an aggregate. This is not a C++
>>> `&', but really something which can have its address taken, and
>>> then act as a pointer to member, for example TYPE :: FIELD can have
>>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
>>> index 402365285cf..e8b5d8fbb73 100644
>>> --- a/gcc/cp/pt.cc
>>> +++ b/gcc/cp/pt.cc
>>> @@ -26602,8 +26602,7 @@ instantiate_decl (tree d, bool defer_ok, bool
>>> expl_inst_class_mem_p)
>>> const_init
>>> = DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (code_pattern);
>>> cp_finish_decl (d, init, /*init_const_expr_p=*/const_init,
>>> - /*asmspec_tree=*/NULL_TREE,
>>> - LOOKUP_ONLYCONVERTING);
>>> + /*asmspec_tree=*/NULL_TREE, 0);
>>> }
>>> if (enter_context)
>>> pop_nested_class ();
>>> diff --git a/gcc/testsuite/g++.dg/cpp0x/explicit15.C
>>> b/gcc/testsuite/g++.dg/cpp0x/explicit15.C
>>> new file mode 100644
>>> index 00000000000..076d8b31631
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/cpp0x/explicit15.C
>>> @@ -0,0 +1,83 @@
>>> +// PR c++/87820
>>> +// { dg-do compile { target c++11 } }
>>> +
>>> +struct A {
>>> + constexpr explicit operator int() const { return 0; }
>>> +};
>>> +
>>> +template<class T>
>>> +void f() {
>>> + A a;
>>> + T t1 = a; // { dg-error "cannot convert" }
>>> + T t2 = {a}; // { dg-error "cannot convert" }
>>> + T t3(a);
>>> + T t4{a};
>>> + T t5 = T(a);
>>> + T t6 = T{a};
>>> + new T(a);
>>> + new T{a};
>>> +}
>>> +
>>> +template<class T>
>>> +void g() {
>>> + T t;
>>> + int n1 = t; // { dg-error "cannot convert" }
>>> + int n2 = {t}; // { dg-error "cannot convert" }
>>> + int n3(t);
>>> + int n4{t};
>>> + int n5 = int(t);
>>> + int n6 = int{t};
>>> + new int(t);
>>> + new int{t};
>>> +}
>>> +
>>> +template void f<int>();
>>> +template void g<A>();
>>> +
>>> +template<class T>
>>> +struct B {
>>> + static constexpr A a{};
>>> + static constexpr T t1 = a; // { dg-error "cannot convert" }
>>> + static constexpr T t2 = {a}; // { dg-error "cannot convert" }
>>> + static constexpr T t4{a}; // { dg-bogus "cannot convert" }
>>> + static constexpr T t5 = T(a);
>>> + static constexpr T t6 = T{a};
>>> +};
>>> +
>>> +template<class T>
>>> +struct C {
>>> + static constexpr T t{};
>>> + static constexpr int n1 = t; // { dg-error "cannot convert" }
>>> + static constexpr int n2 = {t}; // { dg-error "cannot convert" }
>>> + static constexpr int n4{t}; // { dg-bogus "cannot convert" }
>>> + static constexpr int n5 = int(t);
>>> + static constexpr int n6 = int{t};
>>> +};
>>> +
>>> +template struct B<int>;
>>> +template struct C<A>;
>>> +
>>> +#if __cpp_inline_variables
>>> +template<class T>
>>> +struct D {
>>> + static inline A a;
>>> + static inline T t1 = a; // { dg-error "cannot convert" "" { target c++17
>>> } }
>>> + static inline T t2 = {a}; // { dg-error "cannot convert" "" { target
>>> c++17 } }
>>> + static inline T t4{a};
>>> + static inline T t5 = T(a);
>>> + static inline T t6 = T{a};
>>> +};
>>> +
>>> +template<class T>
>>> +struct E {
>>> + static inline T t;
>>> + static inline int n1 = t; // { dg-error "cannot convert" "" { target
>>> c++17 } }
>>> + static inline int n2 = {t}; // { dg-error "cannot convert" "" { target
>>> c++17 } }
>>> + static inline int n4{t};
>>> + static inline int n5 = int(t);
>>> + static inline int n6 = int{t};
>>> +};
>>> +
>>> +template struct D<int>;
>>> +template struct E<A>;
>>> +#endif
>>> diff --git a/gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
>>> b/gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
>>> new file mode 100644
>>> index 00000000000..e82c4bafb04
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
>>> @@ -0,0 +1,78 @@
>>> +// PR c++/102137
>>> +// { dg-do compile { target c++17 } }
>>> +
>>> +template<class T>
>>> +struct A {
>>> + constexpr A() { }
>>> + constexpr A(int) { }
>>> +};
>>> +
>>> +explicit A(...) -> A<int>;
>>> +
>>> +template<template<class> class TT>
>>> +void f() {
>>> + TT x1 = 0; // { dg-error "deduction|no match" }
>>> + TT x2 = {0}; // { dg-error "explicit deduction guide" }
>>> + TT x3(0);
>>> + TT x4{0};
>>> + TT x5;
>>> + new TT(0);
>>> + new TT{0};
>>> + new TT();
>>> + new TT{};
>>> + new TT;
>>> +}
>>> +
>>> +template<class T>
>>> +void g(T t) {
>>> + A a1 = t; // { dg-error "deduction|no match" }
>>> + A a2 = {t}; // { dg-error "explicit deduction guide" }
>>> + A a3(t);
>>> + A a4{t};
>>> + A a5;
>>> + new A(t);
>>> + new A{t};
>>> +}
>>> +
>>> +template void f<A>();
>>> +template void g(int);
>>> +
>>> +template<template<class> class TT>
>>> +struct B {
>>> + static inline TT x1 = 0; // { dg-error "deduction|no match" }
>>> + static inline TT x2 = {0}; // { dg-error "explicit deduction guide" }
>>> + static inline TT x4{0};
>>> + static inline TT x5;
>>> +};
>>> +
>>> +template<class T>
>>> +struct C {
>>> + static inline T t;
>>> + static inline A a1 = t; // { dg-error "deduction|no match" }
>>> + static inline A a2 = {t}; // { dg-error "explicit deduction guide" }
>>> + static inline A a4{t};
>>> + static inline A a5{};
>>> +};
>>> +
>>> +template struct B<A>;
>>> +template struct C<int>;
>>> +
>>> +template<template<class> class TT>
>>> +struct E {
>>> + static constexpr TT x1 = 0; // { dg-error "deduction|no match" }
>>> + static constexpr TT x2 = {0}; // { dg-error "explicit deduction guide" }
>>> + static constexpr TT x4{0};
>>> + static constexpr TT x5{};
>>> +};
>>> +
>>> +template<class T>
>>> +struct F {
>>> + static constexpr T t{};
>>> + static constexpr A a1 = t; // { dg-error "deduction|no match" }
>>> + static constexpr A a2 = {t}; // { dg-error "explicit deduction guide" }
>>> + static constexpr A a4{t};
>>> + static constexpr A a5{};
>>> +};
>>> +
>>> +template struct E<A>;
>>> +template struct F<int>;
>>
>>
>
prev parent reply other threads:[~2022-03-08 22:26 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-04 18:24 Patrick Palka
2022-03-04 22:34 ` Jason Merrill
2022-03-07 14:47 ` Patrick Palka
2022-03-07 19:14 ` Jason Merrill
2022-03-08 15:36 ` Patrick Palka
2022-03-08 16:24 ` Jason Merrill
2022-03-08 18:38 ` Patrick Palka
2022-03-08 20:07 ` Jason Merrill
2022-03-08 21:17 ` Patrick Palka
2022-03-08 22:26 ` Jason Merrill [this message]
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=a673d270-3817-997f-9307-c5e35c11491b@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).