Re: [PATCH][gcc] libgccjit: check result_type in gcc_jit_context_new_unary_op

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2019-07-18 at 14:20 +0000, Andrea Corallo wrote:
> Hi all,
> I've just realized that what we has been done recently for
> gcc_jit_context_new_binary_op should be done also for the unary
> version.
> This patch checks at record time for the result type of
> gcc_jit_context_new_unary_op to be numeric type plus add a testcase
> for the new check.
> 
> make check-jit runs clean
> 
> Is it okay for trunk?
> 
> Bests
>   Andrea
> 
> gcc/jit/ChangeLog
> 2019-07-18  Andrea Corallo <andrea.corallo@arm.com>
> 
> 	* libgccjit.c (gcc_jit_context_new_unary_op): Check result_type
> to be a
> 	numeric type.
> 	* libgccjit.c (gcc_jit_context_new_binary_op): Fix nit in error
> message.
> 
> gcc/testsuite/ChangeLog
> 2019-07-04  Andrea Corallo <andrea.corallo@arm.com>
> 
> 	* jit.dg/test-error-gcc_jit_context_new_unary_op-bad-res-
> type.c:
> 	New testcase.
> 	* jit.dg/test-error-gcc_jit_context_new_binary_op-bad-res-
> type.c:
> 	Fix nit in error message.

Thanks for the patch.  What happens with the existing code if the user
tries to use such a unary op?

> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 23e83e2..bea840f 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -1336,6 +1336,12 @@ gcc_jit_context_new_unary_op (gcc_jit_context *ctxt,
>      "unrecognized value for enum gcc_jit_unary_op: %i",
>      op);
>    RETURN_NULL_IF_FAIL (result_type, ctxt, loc, "NULL result_type");
> +  RETURN_NULL_IF_FAIL_PRINTF3 (
> +    result_type->is_numeric (), ctxt, loc,
> +    "gcc_jit_unary_op %i with operand %s "
> +    "has non-numeric result_type: %s",
> +    op, rvalue->get_debug_string (),
> +    result_type->get_debug_string ());
>    RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue");

The use of "%i" for "op" here isn't as user-friendly as it could be; it
would be ideal to tell the user the enum value.

"op" has already been validated, so why not expose the currently-static 
unary_op_reproducer_strings from jit-recording.c in an internal header,
and use it here with a "%s"?

>    return (gcc_jit_rvalue *)ctxt->new_unary_op (loc, op, result_type,
rvalue);
> @@ -1388,7 +1394,7 @@ gcc_jit_context_new_binary_op (gcc_jit_context
*ctxt,
>    RETURN_NULL_IF_FAIL_PRINTF4 (
>      result_type->is_numeric (), ctxt, loc,
>      "gcc_jit_binary_op %i with operands a: %s b: %s "
> -    "has non numeric result_type: %s",
> +    "has non-numeric result_type: %s",
>      op, a->get_debug_string (), b->get_debug_string (),
>      result_type->get_debug_string ());

Ah, I see there's one of these "%i" for op already.  Given that you're
already fixing a nit here, please make this print "%s", using
binary_op_reproducer_strings from jit-recording.c ("op" has already
been validated).

Thanks
Dave