Re: [PATCH] libgccjit: Add type checks in gcc_jit_block_add_assignment_op

[Antoni Boucher <bouanto@zoho.com>]

David: Ping.

On Thu, 2023-12-21 at 08:36 -0500, Antoni Boucher wrote:
> Hi.
> Here's the updated patch.
> Thanks.
> 
> On Thu, 2023-12-07 at 20:15 -0500, David Malcolm wrote:
> > On Thu, 2023-12-07 at 17:34 -0500, Antoni Boucher wrote:
> > > Hi.
> > > This patch adds checks gcc_jit_block_add_assignment_op to make
> > > sure
> > > it
> > > is only ever called on numeric types.
> > > 
> > > With the previous patch, this might require a change to also
> > > allow
> > > vector types here.
> > > 
> > > Thanks for the review.
> > 
> > Thanks for the patch.
> > 
> > [...snip...]
> > 
> > > @@ -2890,6 +2900,17 @@ gcc_jit_block_add_assignment_op
> > > (gcc_jit_block *block,
> > >      lvalue->get_type ()->get_debug_string (),
> > >      rvalue->get_debug_string (),
> > >      rvalue->get_type ()->get_debug_string ());
> > > +  // TODO: check if it is a numeric vector?
> > > +  RETURN_IF_FAIL_PRINTF3 (
> > > +    lvalue->get_type ()->is_numeric () && rvalue->get_type ()-
> > > > is_numeric (), ctxt, loc,
> > > +    "gcc_jit_block_add_assignment_op %s has non-numeric lvalue
> > > %s
> > > (type: %s)",
> > > +    gcc::jit::binary_op_reproducer_strings[op],
> > > +    lvalue->get_debug_string (), lvalue->get_type ()-
> > > > get_debug_string ());
> > 
> > The condition being tested here should probably just be:
> > 
> >    lvalue->get_type ()->is_numeric ()
> > 
> > since otherwise if the lvalue's type is numeric and the rvalue's
> > type
> > fails to be, then the user would incorrectly get a message about
> > the
> > lvalue.
> > 
> > > +  RETURN_IF_FAIL_PRINTF3 (
> > > +    rvalue->get_type ()->is_numeric () && rvalue->get_type ()-
> > > > is_numeric (), ctxt, loc,
> > > +    "gcc_jit_block_add_assignment_op %s has non-numeric rvalue
> > > %s
> > > (type: %s)",
> > > +    gcc::jit::binary_op_reproducer_strings[op],
> > > +    rvalue->get_debug_string (), rvalue->get_type ()-
> > > > get_debug_string ());
> > 
> > The condition being tested here seems to have a redundant repeated:
> >   && rvalue->get_type ()->is_numeric ()
> > 
> > Am I missing something, or is that a typo?
> > 
> > [...snip...]
> > 
> > The patch is OK otherwise.
> > 
> > Thanks
> > Dave
> > 
> > 
> > 
>