Re: [PATCH] c++: shortcut bad reference bindings [PR94894]
On Mon, 18 Jul 2022, Jason Merrill wrote:
> On 7/18/22 12:59, Patrick Palka wrote:
> > In case of l/rvalue or cv-qual mismatch during reference binding, we try
> > to give more helpful diagnostics by attempting a bad conversion that
> > ignores the mismatch. But in doing so, we may end up instantiating an
> > ill-formed conversion function, something that would otherwise be
> > avoided if we didn't try for the better diagnostic in the first place.
> > We could just give up on producing a good diagnostic in this case, but
> > ideally we could keep the good diagnostics while avoiding unnecessary
> > template instantiations.
> >
> > To that end, this patch adapts the bad conversion shortcutting mechanism
> > from r12-3346-g47543e5f9d1fc5 to handle this situation as well. The main
> > observation from there applies here as well: when there's a strictly
> > viable candidate, we don't care about the distinction between an unviable
> > and non-strictly viable candidate. And in turn it also means we don't
> > really care about the distinction between an invalid and bad conversion.
> > Of course, we don't know whether there's a strictly viable candidate until
> > after the fact, so we still need to keep track of when we "shortcutted"
> > distinguishing between an invalid and bad conversion. This patch adds a
> > new kind of conversion, ck_shortcutted, to represent such conversions.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> >
> > PR c++/94894
> > PR c++/105766
> > PR c++/106201
> >
> > gcc/cp/ChangeLog:
> >
> > * call.cc (enum conversion_kind): Add ck_shortcutted enumerator.
> > (has_next): Return false for it.
> > (reference_binding): Return a ck_shortcutted conversion instead
> > of an actual bad conversion when LOOKUP_SHORTCUT_BAD_CONVS is set.
> > Remove now obsolete early exit for the incomplete target type case.
> > (implicit_conversion_1): Don't mask out LOOKUP_SHORTCUT_BAD_CONVS.
> > (add_function_candidate): Set LOOKUP_SHORTCUT_BAD_CONVS iff
> > shortcut_bad_convs.
> > (missing_conversion_p): Return true for a ck_shortcutted
> > conversion.
> > * cp-tree.h (LOOKUP_SHORTCUT_BAD_CONVS): Define.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/conversion/ref8.C: New test.
> > * g++.dg/conversion/ref9.C: New test.
> > ---
> > gcc/cp/call.cc | 87 --
> > gcc/cp/cp-tree.h | 5 ++
> > gcc/testsuite/g++.dg/conversion/ref8.C | 22 +++
> > gcc/testsuite/g++.dg/conversion/ref9.C | 21 +++
> > 4 files changed, 103 insertions(+), 32 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/conversion/ref8.C
> > create mode 100644 gcc/testsuite/g++.dg/conversion/ref9.C
> >
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index fc98552fda2..ca2f5fbca39 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -59,7 +59,8 @@ enum conversion_kind {
> > ck_ambig,
> > ck_list,
> > ck_aggr,
> > - ck_rvalue
> > + ck_rvalue,
> > + ck_shortcutted,
>
> Please give this a comment explaining how it's used. OK with that change.
Done.
>
> Maybe ck_deferred_bad?
Ah nice, I like this much better than ck_shortcutted. Here's what I
ended up pushing, thanks a lot:
-- >8 --
Subject: [PATCH] c++: shortcut bad reference binding [PR94894]
In case of l/rvalue or cv-qual mismatch during reference binding, we
try to give more helpful diagnostics by computing a bad conversion that
allows the mismatch. But in doing so, we may end up considering and
instantiating a conversion function that could induce a hard error and
in turn cause us to reject otherwise valid code. We could just give up
on producing a better diagnostic here, but ideally we'd preserve the
better diagnostics for invalid code while avoiding unnecessary template
instantiations for valid code.
To that end, this patch adapts the bad conversion shortcutting mechanism
from r12-3346-g47543e5f9d1fc5 to additionally handle this situation.
The main observation from there is that during overload resolution, if we
know we have a strictly viable candidate then we don't need to distinguish
between an unviable and non-strictly viable candidate. Thus we don't
need to distinguish between an invalid and bad conversion either, which
is what this patch exploits. Of course, we don't know whether we have a
strictly viable candidate until after the fact, so we still need to
remember when we deferred distinguishing between an invalid and bad
conversion. This patch adds a special conversion kind ck_deferred_bad
for this purpose.
PR c++/94894
PR c++/105766
PR c++/106201
gcc/cp/ChangeLog:
* call.cc (enum conversion_kind): Add ck_deferred_bad enumerator.
(has_next): Return false for it.
(reference_binding): Return a ck_deferred_bad conversion instead
of an actual bad conversion when
Re: [PATCH] c++: shortcut bad reference bindings [PR94894]
On 7/18/22 12:59, Patrick Palka wrote:
In case of l/rvalue or cv-qual mismatch during reference binding, we try
to give more helpful diagnostics by attempting a bad conversion that
ignores the mismatch. But in doing so, we may end up instantiating an
ill-formed conversion function, something that would otherwise be
avoided if we didn't try for the better diagnostic in the first place.
We could just give up on producing a good diagnostic in this case, but
ideally we could keep the good diagnostics while avoiding unnecessary
template instantiations.
To that end, this patch adapts the bad conversion shortcutting mechanism
from r12-3346-g47543e5f9d1fc5 to handle this situation as well. The main
observation from there applies here as well: when there's a strictly
viable candidate, we don't care about the distinction between an unviable
and non-strictly viable candidate. And in turn it also means we don't
really care about the distinction between an invalid and bad conversion.
Of course, we don't know whether there's a strictly viable candidate until
after the fact, so we still need to keep track of when we "shortcutted"
distinguishing between an invalid and bad conversion. This patch adds a
new kind of conversion, ck_shortcutted, to represent such conversions.
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?
PR c++/94894
PR c++/105766
PR c++/106201
gcc/cp/ChangeLog:
* call.cc (enum conversion_kind): Add ck_shortcutted enumerator.
(has_next): Return false for it.
(reference_binding): Return a ck_shortcutted conversion instead
of an actual bad conversion when LOOKUP_SHORTCUT_BAD_CONVS is set.
Remove now obsolete early exit for the incomplete target type case.
(implicit_conversion_1): Don't mask out LOOKUP_SHORTCUT_BAD_CONVS.
(add_function_candidate): Set LOOKUP_SHORTCUT_BAD_CONVS iff
shortcut_bad_convs.
(missing_conversion_p): Return true for a ck_shortcutted
conversion.
* cp-tree.h (LOOKUP_SHORTCUT_BAD_CONVS): Define.
gcc/testsuite/ChangeLog:
* g++.dg/conversion/ref8.C: New test.
* g++.dg/conversion/ref9.C: New test.
---
gcc/cp/call.cc | 87 --
gcc/cp/cp-tree.h | 5 ++
gcc/testsuite/g++.dg/conversion/ref8.C | 22 +++
gcc/testsuite/g++.dg/conversion/ref9.C | 21 +++
4 files changed, 103 insertions(+), 32 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/conversion/ref8.C
create mode 100644 gcc/testsuite/g++.dg/conversion/ref9.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index fc98552fda2..ca2f5fbca39 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -59,7 +59,8 @@ enum conversion_kind {
ck_ambig,
ck_list,
ck_aggr,
- ck_rvalue
+ ck_rvalue,
+ ck_shortcutted,
Please give this a comment explaining how it's used. OK with that change.
Maybe ck_deferred_bad?
};
/* The rank of the conversion. Order of the enumerals matters; better
@@ -775,7 +776,8 @@ has_next (conversion_kind code)
return !(code == ck_identity
|| code == ck_ambig
|| code == ck_list
- || code == ck_aggr);
+ || code == ck_aggr
+ || code == ck_shortcutted);
}
static conversion *
@@ -1912,18 +1914,38 @@ reference_binding (tree rto, tree rfrom, tree expr,
bool c_cast_p, int flags,
difference in top-level cv-qualification is subsumed by the
initialization itself and does not constitute a conversion. */
+ bool maybe_valid_p = true;
+
/* [dcl.init.ref]
Otherwise, the reference shall be an lvalue reference to a
non-volatile const type, or the reference shall be an rvalue
- reference.
+ reference. */
+ if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto))
+maybe_valid_p = false;
- We try below to treat this as a bad conversion to improve diagnostics,
- but if TO is an incomplete class, we need to reject this conversion
- now to avoid unnecessary instantiation. */
- if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto)
- && !COMPLETE_TYPE_P (to))
-return NULL;
+ /* [dcl.init.ref]
+
+ Otherwise, a temporary of type "cv1 T1" is created and
+ initialized from the initializer expression using the rules for a
+ non-reference copy initialization. If T1 is reference-related to
+ T2, cv1 must be the same cv-qualification as, or greater
+ cv-qualification than, cv2; otherwise, the program is ill-formed. */
+ if (related_p && !at_least_as_qualified_p (to, from))
+maybe_valid_p = false;
+
+ /* We try below to treat an invalid reference binding as a bad conversion
+ to improve diagnostics, but doing so may cause otherwise unnecessary
+ instantiations that can lead to hard error. So during the first pass
+ of overload resolution wherein we shortcut bad conversions,
[PATCH] c++: shortcut bad reference bindings [PR94894]
In case of l/rvalue or cv-qual mismatch during reference binding, we try
to give more helpful diagnostics by attempting a bad conversion that
ignores the mismatch. But in doing so, we may end up instantiating an
ill-formed conversion function, something that would otherwise be
avoided if we didn't try for the better diagnostic in the first place.
We could just give up on producing a good diagnostic in this case, but
ideally we could keep the good diagnostics while avoiding unnecessary
template instantiations.
To that end, this patch adapts the bad conversion shortcutting mechanism
from r12-3346-g47543e5f9d1fc5 to handle this situation as well. The main
observation from there applies here as well: when there's a strictly
viable candidate, we don't care about the distinction between an unviable
and non-strictly viable candidate. And in turn it also means we don't
really care about the distinction between an invalid and bad conversion.
Of course, we don't know whether there's a strictly viable candidate until
after the fact, so we still need to keep track of when we "shortcutted"
distinguishing between an invalid and bad conversion. This patch adds a
new kind of conversion, ck_shortcutted, to represent such conversions.
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?
PR c++/94894
PR c++/105766
PR c++/106201
gcc/cp/ChangeLog:
* call.cc (enum conversion_kind): Add ck_shortcutted enumerator.
(has_next): Return false for it.
(reference_binding): Return a ck_shortcutted conversion instead
of an actual bad conversion when LOOKUP_SHORTCUT_BAD_CONVS is set.
Remove now obsolete early exit for the incomplete target type case.
(implicit_conversion_1): Don't mask out LOOKUP_SHORTCUT_BAD_CONVS.
(add_function_candidate): Set LOOKUP_SHORTCUT_BAD_CONVS iff
shortcut_bad_convs.
(missing_conversion_p): Return true for a ck_shortcutted
conversion.
* cp-tree.h (LOOKUP_SHORTCUT_BAD_CONVS): Define.
gcc/testsuite/ChangeLog:
* g++.dg/conversion/ref8.C: New test.
* g++.dg/conversion/ref9.C: New test.
---
gcc/cp/call.cc | 87 --
gcc/cp/cp-tree.h | 5 ++
gcc/testsuite/g++.dg/conversion/ref8.C | 22 +++
gcc/testsuite/g++.dg/conversion/ref9.C | 21 +++
4 files changed, 103 insertions(+), 32 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/conversion/ref8.C
create mode 100644 gcc/testsuite/g++.dg/conversion/ref9.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index fc98552fda2..ca2f5fbca39 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -59,7 +59,8 @@ enum conversion_kind {
ck_ambig,
ck_list,
ck_aggr,
- ck_rvalue
+ ck_rvalue,
+ ck_shortcutted,
};
/* The rank of the conversion. Order of the enumerals matters; better
@@ -775,7 +776,8 @@ has_next (conversion_kind code)
return !(code == ck_identity
|| code == ck_ambig
|| code == ck_list
- || code == ck_aggr);
+ || code == ck_aggr
+ || code == ck_shortcutted);
}
static conversion *
@@ -1912,18 +1914,38 @@ reference_binding (tree rto, tree rfrom, tree expr,
bool c_cast_p, int flags,
difference in top-level cv-qualification is subsumed by the
initialization itself and does not constitute a conversion. */
+ bool maybe_valid_p = true;
+
/* [dcl.init.ref]
Otherwise, the reference shall be an lvalue reference to a
non-volatile const type, or the reference shall be an rvalue
- reference.
+ reference. */
+ if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto))
+maybe_valid_p = false;
- We try below to treat this as a bad conversion to improve diagnostics,
- but if TO is an incomplete class, we need to reject this conversion
- now to avoid unnecessary instantiation. */
- if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto)
- && !COMPLETE_TYPE_P (to))
-return NULL;
+ /* [dcl.init.ref]
+
+ Otherwise, a temporary of type "cv1 T1" is created and
+ initialized from the initializer expression using the rules for a
+ non-reference copy initialization. If T1 is reference-related to
+ T2, cv1 must be the same cv-qualification as, or greater
+ cv-qualification than, cv2; otherwise, the program is ill-formed. */
+ if (related_p && !at_least_as_qualified_p (to, from))
+maybe_valid_p = false;
+
+ /* We try below to treat an invalid reference binding as a bad conversion
+ to improve diagnostics, but doing so may cause otherwise unnecessary
+ instantiations that can lead to hard error. So during the first pass
+ of overload resolution wherein we shortcut bad conversions, instead just
+ produce a special conversion that we'll use to indicate we might need to
+ perform a second pass if there's no strictly viable candidate. */
+ if
