| Message ID | alpine.DEB.2.20.1706271400410.15648@digraph.polyomino.org.uk |
|---|---|
| State | New |
| Headers | show |
27.06.2017 16:01 Joseph Myers <joseph@codesourcery.com> wrote: > [...] > +#define DO_NUMBER(d, v) \ > + do \ > + { \ > + digits = d > width ? d : width; \ > + number_value = v; \ > + goto do_number; \ > + } \ > + while (0) I know it's a little late to comment this as the patch is already committed but wouldn't a simple compound expression do the work? Like this: #define DO_NUMBER(d, v) \ { \ digits = d > width ? d : width; \ number_value = v; \ goto do_number; \ } I mean without the surrounding do...while (0). > +#define DO_NUMBER_SPACEPAD(d, v) \ > + do \ > + { \ > + digits = d > width ? d : width; \ > + number_value = v; \ > + goto do_number_spacepad; \ > + } \ > + while (0) Likewise. Just asking, I haven't got GCC 8 to test it. Feel free to ignore my comment if it is not worth fixing. Your patch is not harmful, though. Regards, Rafal
Rafal Luzynski wrote:
> wouldn't a simple compound expression do the work?
It's better to use do-while in case the macro is used as a then-part of an
if-statement, where a simple compound expression would cause a syntax error.
FWIW, the Gnulib copy of strftime has been using do-while for some time.
> Dnia 29 czerwiec 2017 o 00:56 Paul Eggert <eggert@cs.ucla.edu> napisał(a): > > > Rafal Luzynski wrote: > > wouldn't a simple compound expression do the work? > > It's better to use do-while in case the macro is used as a then-part of an > if-statement, where a simple compound expression would cause a syntax error. Wouldn't it expand to: if (modifier == L_('O')) goto bad_format; else { digits = 1 > width ? 1 : width; number_value = tp->tm_year + TM_YEAR_BASE; goto do_number; }; I can't see a syntax error unless GCC 8 is different. > FWIW, the Gnulib copy of strftime has been using do-while for some time. As I said before, this is not a huge difference and probably even the binary code is not different. Not worth fixing if you like the code as it is. Regards, Rafal
Rafal Luzynski wrote:
> Wouldn't it expand to:
Sure, but the problem occurs if the macro is used as an if statement's
then-part. Your example involved an else-part.
29.06.2017 03:08 Paul Eggert <eggert@cs.ucla.edu> wrote: > > > Rafal Luzynski wrote: > > Wouldn't it expand to: > > Sure, but the problem occurs if the macro is used as an if statement's > then-part. Your example involved an else-part. OK, you are right. Then-part rather than else-part and after thinking a little more I've found the syntax error you were talking about. Sorry for this off-topic, late night is not the best moment to think about C syntax. Regards, Rafal
diff --git a/time/strftime_l.c b/time/strftime_l.c index 439b971..b5ba9ca 100644 --- a/time/strftime_l.c +++ b/time/strftime_l.c @@ -715,12 +715,22 @@ __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format, format_char = *f; switch (format_char) { -#define DO_NUMBER(d, v) \ - digits = d > width ? d : width; \ - number_value = v; goto do_number -#define DO_NUMBER_SPACEPAD(d, v) \ - digits = d > width ? d : width; \ - number_value = v; goto do_number_spacepad +#define DO_NUMBER(d, v) \ + do \ + { \ + digits = d > width ? d : width; \ + number_value = v; \ + goto do_number; \ + } \ + while (0) +#define DO_NUMBER_SPACEPAD(d, v) \ + do \ + { \ + digits = d > width ? d : width; \ + number_value = v; \ + goto do_number_spacepad; \ + } \ + while (0) case L_('%'): if (modifier != 0)
Building with current GCC mainline fails with: strftime_l.c: In function '__strftime_internal': strftime_l.c:719:4: error: macro expands to multiple statements [-Werror=multistatement-macros] digits = d > width ? d : width; \ ^ strftime_l.c:1260:6: note: in expansion of macro 'DO_NUMBER' DO_NUMBER (1, tp->tm_year + TM_YEAR_BASE); ^~~~~~~~~ strftime_l.c:1259:4: note: some parts of macro expansion are not guarded by this 'else' clause else ^~~~ In fact this particular instance is harmless; the code looks like: if (modifier == L_('O')) goto bad_format; else DO_NUMBER (1, tp->tm_year + TM_YEAR_BASE); and because of the goto, it doesn't matter that part of the expansion isn't under the "else" conditional. But it's also clearly bad style to rely on that. This patch changes DO_NUMBER and DO_NUMBER_SPACEPAD to use do { } while (0) to avoid such problems. Tested (full testsuite) for x86_64 (GCC 6); testing with build-many-glibcs.py with GCC mainline, in conjunction with my libgcc patch <https://gcc.gnu.org/ml/gcc-patches/2017-06/msg02032.html>. I intend to commit if the build-many-glibcs.py testing passes. 2017-06-27 Joseph Myers <joseph@codesourcery.com> * time/strftime_l.c (DO_NUMBER): Define using do { } while (0). (DO_NUMBER_SPACEPAD): Likewise.