| Message ID | c3cd9217-c14b-7e63-5d0c-b7304fa328e7@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, 07 Nov 2018, Adhemerval Zanella wrote: >On 29/10/2018 09:16, Gabriel F. T. Gomes wrote: > >> Additional note for review: >> >> - Reviewing the changes from vfscanf.c to vfscanf-internal.c in the >> original patch would be vey hard, because git doesn't detect the >> filename change. To make review a little easier, I did as Zack did >> and manually edited the diff. I'll reply to this thread and attach >> the original patch if someone wants to apply it. >> (ping me if I forget it) > >I would advise to avoid it, it is error-prone and we do have tools to >handle it. One option is to use -C and -M git options with a higher s >imilarity index, for instance on this patch using: > >git format-patch -M90% -C -1 > >it generates the expected result: It does, indeed. Thanks for the explanation. I'll use it from now on. >> Signed-off-by: Zack Weinberg <zackw@panix.com> >> Signed-off-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br> > >As Florian has said, we don't use DCO, but copyright assignments. Ack, removed. >> +/* Flags for __vfscanf_internal and __vfwscanf_internal. */ >> +#define SCANF_LDBL_IS_DBL 0x0001 >> +#define SCANF_ISOC99_A 0x0002 > >As before, please describe what actually the flag does. Ack, how about the following text? +/* Flags for __vfscanf_internal and __vfwscanf_internal. + + SCANF_LDBL_IS_DBL indicates whether long double values are to be + handled as having the same format as double, in which case the flag + should be set to one, or as another format, otherwise. + + SCANF_ISOC99_A, when set to one, indicates that the ISO C99 or POSIX + behavior of the scanf functions is to be used, i.e. automatic + allocation for input strings with %as, %aS and %a[, a GNU extension, + is disabled. This is the behavior that the __isoc99_scanf family of + functions use. When the flag is set to zero, automatic allocation is + enabled. */ +#define SCANF_LDBL_IS_DBL 0x0001 +#define SCANF_ISOC99_A 0x0002 >> +#define LDBL_DISTINCT (__glibc_likely ((mode_flags & SCANF_LDBL_IS_DBL) == 0)) >> +#define USE_ISOC99_A (__glibc_likely (mode_flags & SCANF_ISOC99_A)) > >Do we really gain anything using these defines? I tend to frown on macros >that use internal named variables instead of set them as arguments. Also >this is quite short, I think it is more readable to just use the comparison >directly. I don't have a strong opinion about it, but it does look good with the comparison directly where it is used. Fixed as suggested. >> +/* Same as compat_symbol, ldbl_compat_symbol is not to be used outside >> + '#if SHLIB_COMPAT' statement and should fail if it is. */ >> +# define ldbl_compat_symbol(lib, local, symbol, version) ... > >Why not use an _Static_assert(0, "ldbl_compat_symbol should be used inside SHLIB_COMPAT")? No reason other than I wasn't aware of _Static_assert. Fixed as suggested. I'll send a new version when I finish reviewing the whole patch set. Thanks for the careful review.
diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf-internal.c similarity index 98% copy from stdio-common/vfscanf.c copy to stdio-common/vfscanf-internal.c index 1ce836a324..1ae37a2a11 100644 --- a/stdio-common/vfscanf.c +++ b/stdio-common/vfscanf-internal.c @@ -1,4 +1,5 @@ -/* Copyright (C) 1991-2018 Free Software Foundation, Inc. +/* Internal functions for the *scanf* implementation. + Copyright (C) 1991-2018 Free Software Foundation, Inc. This file is part of the GNU C Library. [...]