[PATCH v2] c++: Make convert_like complain about bad ck_ref_bind again [PR95789]
Jason Merrill
jason@redhat.com
Sat Jul 11 14:32:55 GMT 2020
On 7/8/20 4:35 PM, Marek Polacek wrote:
> On Fri, Jul 03, 2020 at 05:24:34PM -0400, Jason Merrill via Gcc-patches wrote:
>> On 6/22/20 10:09 PM, Marek Polacek wrote:
>>> convert_like issues errors about bad_p conversions at the beginning
>>> of the function, but in the ck_ref_bind case, it only issues them
>>> after we've called convert_like on the next conversion.
>>>
>>> This doesn't work as expected since r10-7096 because when we see
>>> a conversion from/to class type in a template, we return early, thereby
>>> missing the error, and a bad_p conversion goes by undetected. That
>>> made the attached test to compile even though it should not.
>>
>> Hmm, why isn't there an error at instantiation time?
>
> We threw away the result: we're called from
>
> 12213 if (complain & tf_error)
> 12214 {
> 12215 if (conv)
> 12216 convert_like (conv, expr, complain);
> ...
> 12228 return error_mark_node;
>
> and convert_like never saw converting this->f to B& again when instantiating.
>
>> Though giving an error at template parsing time is definitely preferable.
>
> Yup.
>
>>> I had thought that I could just move the ck_ref_bind/bad_p errors
>>> above to the rest of them, but that regressed diagnostics because
>>> expr then wasn't converted yet by the nested convert_like_real call.
>>
>> Yeah, the early section is really just for scalar conversions.
>>
>> It would probably be good to do normal processing for all other bad
>> conversions and only afterward build the IMPLICIT_CONV_EXPR if we aren't
>> returning error_mark_node.
>
> Ok, so that if we add more bad_p errors, we won't run into this again.
>
> Unfortunately it's a bit ugly. I could introduce a RETURN macro to
> use RETURN (expr); instead of what I have now, but it wouldn't be simply
> "conv_expr ? conv_expr : expr", because if we have error_mark_node, we
> want to return that, not conv_expr. Does that seem worth it?
> (I wish I could at least use the op0 ?: op1 GNU extension.)
Or introduce a wrapper function that handles IMPLICIT_CONV_EXPR?
> I've also added another test.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> -- >8 --
> convert_like issues errors about bad_p conversions at the beginning
> of the function, but in the ck_ref_bind case, it only issues them
> after we've called convert_like on the next conversion.
>
> This doesn't work as expected since r10-7096 because when we see
> a conversion from/to class type in a template, we return early, thereby
> missing the error, and a bad_p conversion goes by undetected. That
> made the attached test to compile even though it should not.
>
> I had thought that I could just move the ck_ref_bind/bad_p errors
> above to the rest of them, but that regressed diagnostics because
> expr then wasn't converted yet by the nested convert_like_real call.
>
> So, for bad_p conversions, do the normal processing, but still return
> the IMPLICIT_CONV_EXPR to avoid introducing trees that the template
> processing can't handle well.
>
> gcc/cp/ChangeLog:
>
> PR c++/95789
> * call.c (convert_like_real): Do the normal processing for
> conversion that are bad_p. Return the IMPLICIT_CONV_EXPR
> instead of EXPR if we're processing a bad_p conversion in
> a template.
>
> gcc/testsuite/ChangeLog:
>
> PR c++/95789
> * g++.dg/conversion/ref4.C: New test.
> * g++.dg/conversion/ref5.C: New test.
> ---
> gcc/cp/call.c | 47 +++++++++++++++-----------
> gcc/testsuite/g++.dg/conversion/ref4.C | 22 ++++++++++++
> gcc/testsuite/g++.dg/conversion/ref5.C | 14 ++++++++
> 3 files changed, 64 insertions(+), 19 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/conversion/ref4.C
> create mode 100644 gcc/testsuite/g++.dg/conversion/ref5.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 5341a572980..65565fc90a8 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -7400,12 +7400,19 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> so use an IMPLICIT_CONV_EXPR for this conversion. We would have
> created such codes e.g. when calling a user-defined conversion
> function. */
> + tree conv_expr = NULL_TREE;
> if (processing_template_decl
> && convs->kind != ck_identity
> && (CLASS_TYPE_P (totype) || CLASS_TYPE_P (TREE_TYPE (expr))))
> {
> - expr = build1 (IMPLICIT_CONV_EXPR, totype, expr);
> - return convs->kind == ck_ref_bind ? expr : convert_from_reference (expr);
> + conv_expr = build1 (IMPLICIT_CONV_EXPR, totype, expr);
> + if (convs->kind != ck_ref_bind)
> + conv_expr = convert_from_reference (conv_expr);
> + if (!convs->bad_p)
> + return conv_expr;
> + /* Do the normal processing to give the bad_p errors. But we still
> + need to return the IMPLICIT_CONV_EXPR, unless we're returning
> + error_mark_node. */
> }
>
> switch (convs->kind)
> @@ -7465,7 +7472,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> TARGET_EXPR_LIST_INIT_P (expr) = true;
> TARGET_EXPR_DIRECT_INIT_P (expr) = direct;
> }
> - return expr;
> + return conv_expr ? conv_expr : expr;
> }
>
> /* We don't know here whether EXPR is being used as an lvalue or
> @@ -7488,7 +7495,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> TARGET_EXPR_LIST_INIT_P (expr) = true;
> }
>
> - return expr;
> + return conv_expr ? conv_expr : expr;
> }
> case ck_identity:
> if (BRACE_ENCLOSED_INITIALIZER_P (expr))
> @@ -7513,7 +7520,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> continue to warn about uses of EXPR as an integer, rather than as a
> pointer. */
> expr = build_int_cst (totype, 0);
> - return expr;
> + return conv_expr ? conv_expr : expr;
> case ck_ambig:
> /* We leave bad_p off ck_ambig because overload resolution considers
> it valid, it just fails when we try to perform it. So we need to
> @@ -7585,7 +7592,8 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> field = next_initializable_field (DECL_CHAIN (field));
> CONSTRUCTOR_APPEND_ELT (vec, field, size_int (len));
> tree new_ctor = build_constructor (totype, vec);
> - return get_target_expr_sfinae (new_ctor, complain);
> + return (conv_expr ? conv_expr
> + : get_target_expr_sfinae (new_ctor, complain));
> }
>
> case ck_aggr:
> @@ -7598,14 +7606,14 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> imag = perform_implicit_conversion (TREE_TYPE (totype),
> imag, complain);
> expr = build2 (COMPLEX_EXPR, totype, real, imag);
> - return expr;
> + return conv_expr ? conv_expr : expr;
> }
> expr = reshape_init (totype, expr, complain);
> expr = get_target_expr_sfinae (digest_init (totype, expr, complain),
> complain);
> if (expr != error_mark_node)
> TARGET_EXPR_LIST_INIT_P (expr) = true;
> - return expr;
> + return conv_expr ? conv_expr : expr;
>
> default:
> break;
> @@ -7634,19 +7642,19 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> }
>
> if (! MAYBE_CLASS_TYPE_P (totype))
> - return expr;
> + return conv_expr ? conv_expr : expr;
>
> /* Don't introduce copies when passing arguments along to the inherited
> constructor. */
> if (current_function_decl
> && flag_new_inheriting_ctors
> && DECL_INHERITED_CTOR (current_function_decl))
> - return expr;
> + return conv_expr ? conv_expr : expr;
>
> if (TREE_CODE (expr) == TARGET_EXPR
> && TARGET_EXPR_LIST_INIT_P (expr))
> /* Copy-list-initialization doesn't actually involve a copy. */
> - return expr;
> + return conv_expr ? conv_expr : expr;
>
> /* Fall through. */
> case ck_base:
> @@ -7657,7 +7665,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> /* Build an expression for `*((base*) &expr)'. */
> expr = convert_to_base (expr, totype,
> !c_cast_p, /*nonnull=*/true, complain);
> - return expr;
> + return conv_expr ? conv_expr : expr;
> }
>
> /* Copy-initialization where the cv-unqualified version of the source
> @@ -7686,7 +7694,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> maybe_inform_about_fndecl_for_bogus_argument_init (fn, argnum);
> }
>
> - return build_cplus_new (totype, expr, complain);
> + return conv_expr ? conv_expr : build_cplus_new (totype, expr, complain);
>
> case ck_ref_bind:
> {
> @@ -7821,16 +7829,16 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> expr = cp_convert (build_pointer_type (TREE_TYPE (ref_type)),
> expr, complain);
> /* Convert the pointer to the desired reference type. */
> - return build_nop (ref_type, expr);
> + return conv_expr ? conv_expr : build_nop (ref_type, expr);
> }
>
> case ck_lvalue:
> - return decay_conversion (expr, complain);
> + return conv_expr ? conv_expr : decay_conversion (expr, complain);
>
> case ck_fnptr:
> /* ??? Should the address of a transaction-safe pointer point to the TM
> clone, and this conversion look up the primary function? */
> - return build_nop (totype, expr);
> + return conv_expr ? conv_expr : build_nop (totype, expr);
>
> case ck_qual:
> /* Warn about deprecated conversion if appropriate. */
> @@ -7845,11 +7853,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> if (convs->base_p)
> expr = convert_to_base (expr, totype, !c_cast_p,
> /*nonnull=*/false, complain);
> - return build_nop (totype, expr);
> + return conv_expr ? conv_expr : build_nop (totype, expr);
>
> case ck_pmem:
> - return convert_ptrmem (totype, expr, /*allow_inverse_p=*/false,
> + expr = convert_ptrmem (totype, expr, /*allow_inverse_p=*/false,
> c_cast_p, complain);
> + return conv_expr ? conv_expr : expr;
>
> default:
> break;
> @@ -7866,7 +7875,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> else
> expr = cp_convert (totype, expr, complain);
>
> - return expr;
> + return conv_expr ? conv_expr : expr;
> }
>
> /* ARG is being passed to a varargs function. Perform any conversions
> diff --git a/gcc/testsuite/g++.dg/conversion/ref4.C b/gcc/testsuite/g++.dg/conversion/ref4.C
> new file mode 100644
> index 00000000000..464a4cf6c0f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/conversion/ref4.C
> @@ -0,0 +1,22 @@
> +// PR c++/95789
> +// { dg-do compile { target c++11 } }
> +
> +struct B {
> + int n;
> +};
> +
> +template <typename T>
> +struct A {
> + B& get() const { return f; } // { dg-error "binding reference" }
> +
> + B f;
> +};
> +
> +int main() {
> + A<int> a;
> + a.f = {};
> +
> + a.get().n = 10;
> + if (a.f.n != 0)
> + __builtin_abort();
> +}
> diff --git a/gcc/testsuite/g++.dg/conversion/ref5.C b/gcc/testsuite/g++.dg/conversion/ref5.C
> new file mode 100644
> index 00000000000..0042acd0670
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/conversion/ref5.C
> @@ -0,0 +1,14 @@
> +// PR c++/96104
> +
> +template <typename T> void fn(T &);
> +class E {};
> +struct F {
> + template <typename T> void mfn(T t) { t, fn(E()); } // { dg-error "cannot bind non-const lvalue reference" }
> +};
> +int
> +main()
> +{
> + E e;
> + F f;
> + f.mfn(e);
> +}
>
> base-commit: 760df6d296b8fc59796f42dca5eb14012fbfa28b
>
More information about the Gcc-patches
mailing list