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.cora...@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.cora...@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
