Re: [PATCH 2/3] Add default implementation of fenv.h and all methods

On Wed, Jul 31, 2019 at 6:58 PM Howland, Craig D. - US via newlib <
[email protected]> wrote:

> > From: [email protected] <[email protected]> on
> behalf of Joel Sherrill <[email protected]>
> > Sent: Wednesday, July 31, 2019 7:13 PM
> > To: [email protected]
> > Cc: Joel Sherrill
> > Subject: [PATCH 2/3] Add default implementation of fenv.h and all methods
> >
> >         The default implementation of the fenv.h methods return
> >         -EOPNOTSUPP.
> >
> >         The intention of the new fenv.h is that it be portable
> >         and that architectures provide their own implementation
> >         of sys/fenv.h.
> > ...
>
> The RISC-V-specific things would probably be best removed from
> libc/include/sys/fenv.h, leaving the original one in machine (unlike the
> primary fenv.h, which will no longer need to be under machine, as you
> noted.)  At a glance, using shorthand:
> -#define FE_TONEAREST_MM 0x00000004
> -#define FE_RMODE_MASK   0x7
> (Put another way, it should only have the bare-bones items defined in C
> and POSIX.)
>

Thanks. I hadn't reviewed it against C/POSIX yet and should have.

Do you think there should be comments about constants and types in the
default sys/fenv.h or just a blanket, this is the POSIX minimum that a port
has to define

I honestly was worried the structure of this work or the regenerated stuff
would be awfully broken. I am thrilled to get meaty technical comments.


>
> In general:
> +  return -EOPNOTSUPP;
>      ENOTSUP, not EOPNOTSUPP.  (The latter appears first, so I'm guessing
> you searched on NOTSUP and just copied the wrong one).
>
>
Oh. My grep was included OP and missed ENOTSUPP


> I'd suggest comments in the .c files to say they are non-functional
> placeholders, even though that should be obvious.
>

Gotcha. Will do.

>
> While the fenv/Makefile stuff does set up the chewout files, there is no
> man page stuff in the C files to get.
>

I was planning to add it.  Especially since there isn't much to add.

>
> libm/Makefile.am I think also needs:
> -libm.dvi: targetdep.tex math/stmp-def complex/stmp-def
> +libm.dvi: targetdep.tex math/stmp-def complex/stmp-def fenv/stmp-def
> +fenv/stmp-def: stmp-targetdep ; @true
> (It is set up poorly for maintenance, as ideally you'd want these to be
> automatic from the SUBDIRS line.)
>

Thanks. I had no idea on this.

I will fix all this and submit a v2.

>
> Craig