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). > Finally I noticed I had a mismatch in the _sprintfkf.h include file, and I > fixed that. What does this mean? 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. > * 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). 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. > -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. Segher
