Re: [PATCH 3/5] sim: Add partial support for IEEE 754-2008

[Mike Frysinger <vapier@gentoo.org>]

On 20 May 2021 13:15, Faraz Shahbazker wrote:

just nits

> --- a/sim/common/sim-fpu.c
> +++ b/sim/common/sim-fpu.c
>  
> +/* NaN handling specific to min/max operations.  */
> +
> +INLINE_SIM_FPU (int)
> +sim_fpu_minmax_nan (sim_fpu *f, const sim_fpu *l, const sim_fpu *r)
> +{
> +  if (sim_fpu_is_snan (l)
> +      || sim_fpu_is_snan (r)
> +      || sim_fpu_is_ieee754_1985 ())
> +    return sim_fpu_op_nan (f, l, r);
> +  else
> +    /* if sim_fpu_is_ieee754_2008()
> +       && ((sim_fpu_is_qnan (l) || sim_fpu_is_qnan (r))) */

i know it's a comment, but we want the correct style either way

    /* if (sim_fpu_is_ieee754_2008 ()
       && (sim_fpu_is_qnan (l) || sim_fpu_is_qnan (r))) */

> +    {
> +      /* In IEEE754-2008:
> +       * "minNum/maxNum is ... the canonicalized number if one
> +       * operand is a number and the other a quiet NaN." */

don't want the leading * in multiline comment blocks.
two spaces before the trailing */.

> +      if (sim_fpu_is_qnan (l))
> +	*f = *r;
> +      else if (sim_fpu_is_qnan (r))
> +	*f = *l;

should the else be a comment too ?

> +INLINE_SIM_FPU (int)
> +sim_fpu_is_un (const sim_fpu *l,
> +    const sim_fpu *r)
>
> +INLINE_SIM_FPU (int)
> +sim_fpu_is_or (const sim_fpu *l,
> +    const sim_fpu *r)
>
> +INLINE_SIM_FPU (int)
> +sim_fpu_un (int *is,
> +             const sim_fpu *l,
> +             const sim_fpu *r)

bad indentation on these.  you could just unwrap them.

> +{
> +  if (sim_fpu_is_nan (l) || sim_fpu_is_nan (r))
> +  {
> +    *is = 1;
> +    return 0;
> +  }

bad indentation

> +  /* Invert result */

period & two spaces at the end

> +  *is = (*is) ? 0 : 1;

simpler ?
	*is = !*is;

> +INLINE_SIM_FPU(int)
> +sim_fpu_classify(const sim_fpu *f)

space before the (
-mike