On Fri, Feb 26, 2021 at 07:32:25PM -0500, Michael Meissner wrote: > 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: > > > 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.
Just don't. Please. If you optimise your patches for reviewer time (and, assume the reviewer knows nothing about your patch yet), you'll end up with a nice series that is easy to write commit messages (and changelogs) for as well. Try it some time, you'll be amazed! > > > 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. Okay. This was clear after reading the patch, but the message should help reading the patch, not the other way around! > > > * 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. I don't see how this is true. I cannot see what is new code and what is not anyway, so it is hard to look deeper. You can just do #if __FLOAT128__ && ENABLE_DECIMAL_FLOAT ... #endif (or similar, whichever macros you need exactly) around the code that cannot be built without both these things. > > > -#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. If you have sprintf, you have <stdio.h>. The C standard demands it. Segher
