Re: [PATCH v2 2/8] Add __vfscanf_internal and __vfwscanf_internal with flags arguments.

["Gabriel F. T. Gomes" <gabriel at inconstante dot eti dot br>]

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.