On Fri, Feb 26, 2021 at 04:36:20PM -0600, Segher Boessenkool wrote:
> On Fri, Feb 26, 2021 at 01:33:41AM -0500, Michael Meissner wrote:
> > Honor --disable-decimal-float in building _Float128 support.
> >
> > Joseph Myers reported that my previous patch to add conversions between
> > _Float128 and the Decimal types was still being built even if GCC was
> > configured with the --disable-decimal-float option. This patch fixes that
> > by
> > only building the conversion functions if both _Float128 and Decimal support
> > are built into GCC.
>
> Okay.
>
> > In addition, I removed the dependency on the target having a valid stdio.h
> > file
> > to declare sprintf in building _sprintfkf.c.
>
> In the future, do not do multiple independent things in one patch; this
> makes it harder than necessary to review. It also makes it harder for
> you to write good commit messages. With Git it is easy to break out,
> join, reorder, or transmogrify patches (see the -p option to various
> commands, for example).
Joseph reported three bugs in the report, so I implemented them in a single
patch.
> > Finally I noticed I had a mismatch in the _sprintfkf.h include file, and I
> > fixed that.
>
> What does this mean?
The include file _sprintfkf.h had a prototype of:
extern int __sprintfkf (char *restrict, const char *restrict, ...);
while the function in _sprintfkf.c actually was defined:
int __sprintfkf (char *restrict string,
const char *restrict format,
_Float128 number)
Since _sprintfkf.c did not include _sprintfkf.h, I didn't notice that these
things were different.
In fixing the bug to eliminate stdio.h, I noticed that we got a function
declared without a previous declaration message, so I included the include
file, and then noticed that the calling signature was different.
> A reviewer needs to know what the patch is supposed to do. You also
> should say that in the commit message (which ideally your mail message
> _is_ of course).
>
> > * config.host (powerpc*-*-linux): Move the Decimal/_Float128
> > * conversions into t-float128-dec.
>
> Stray bullet.
Thanks.
> > * config/rs6000/t-float128 (fp128_dec_funcs): Move to
> > t-float128-dec.
> > (fp128_decstr_funcs): Move to t-float128-dec.
> > (ibm128_dec_funcs): Move to t-float128-dec.
> > (fp128_dec_objs): Move to t-float128-dec.
> > (fp128_decstr_objs): Move to t-float128-dec.
> > (ibm128_dec_objs): Move to t-float128-dec.
> > (FP128_CFLAGS_DECIMAL): Move to t-float128-dec.
> > (IBM128_CFLAGS_DECIMAL): Move to t-float128-dec.
> > * config/rs6000/t-float128-dec: New file.
>
> You don't say this in the mesasage, either.
>
> If you split code movements like this into a separate patch (at the
> start of the resulting series), it is *much* easier to review. Even
> more so if you made any changes to the code at the same time (not the
> case here, says the changelog).
>
> So, why did this move? I could try to find out, but you could just say,
> much less work :-)
>
> > * configure.ac (libgcc_cv_powerpc_float128_dec): New variable, set
> > to yes if both _Float128 and Decimal support are available.
> > * configure: Regenerate.
>
> Can't you just #ifdef the code? This is much harder to get (subtly)
> wrong. Also you then do not need a name for a complex subject, so you
> won't do a terrible name either. (Well, #if ENABLE_DECIMAL_FLOAT, it
> is defined to 0 instead of being undef'ed).
Not really. The problem is these functions are buried within dfp-bit.c. You
need to change the Makefile not to build these functions. I could use make ifs
to suppress setting these variables. I tend to think having a separate t-*
file is cleaner than a bug of make ifs within a single t-* file.
> Changelogs should not explain the code btw, do that in the code itself,
> or in the commit message.
>
> > -#include <stdio.h>
>
> > +extern int sprintf (char *restrict, const char *restrict, ...);
>
> I'm not sure that is an improvement at all. Why would it be? The point
> is not to not include standard headers (although, for free-standing you
> need that); the point is not to depend on external code, in libgcc.
The idea is you want to be able to compile code that calls sprintf. If you
don't have a stdio.h, the function will not compile. If it compiles, and is
used, it will generate an error at link time.
> > -extern int __sprintfkf (char *restrict, const char *restrict, ...);
> > +extern int __sprintfkf (char *restrict, const char *restrict, _Float128);
>
> This is a separate bug fix, so should be separate.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
