On 07/24/2017 04:06 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Commit 1792d7d0 was written because PRId64 expands to non-portable >> crap for some libc, and we had testsuite failures on Mac OS as a >> result. This in turn makes it difficult to rely on the obvious >> conversions of 64-bit values into JSON, requiring things such as >> casting int64_t to long long so we can use a reliable %lld and >> still keep -Wformat happy. So now it's time to fix that. >>
>> +case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in
>> + '' | *"$nl"* ) error_exit "can't determine value of PRId64" ;;
>> + *_ld | *_lld | *_I64d | *_qd) ;;
>> + *) error_exit "unexepected value of PRId64, please add %$(strings $TMPE
>> |
>> + sed -n s/^UnLiKeLyToClAsH_//p) support to json-lexer.c" ;;
>> +esac
>> +
>
> Why is this easier or more robust than examining output of the
> preprocessor? Hmm, you explain it in the commit message. I think you
> should also (briefly!) explain it in the "Sadly" comment.
Okay. (Something along the lines of: We can't guarantee if the
preprocessor will produce "ld" or "l" "d", nor even if the expansion
will occur on the same line as any marker)
I also wonder if I should anchor some \n in the magic bytes being
searched for in the binary, so that if 'strings' fails (which may indeed
be the case for a mingw binary), then falling back to raw grep may also
have a chance. But first, I'm hoping to get some patchew feedback first
if one of the build platforms has problems with the current attempt.
>> +++ b/qobject/json-lexer.c
>> @@ -31,7 +31,7 @@
>> *
>> * Extension for vararg handling in JSON construction:
>> *
>> - * %((l|ll|I64)?d|[ipsf])
>> + * %(PRI[du]64|(l|ll)?[ud]|[ipsf])
>
> Confusing. The lexer accepts more than that, but parse_escape() filters
> it out. Need a comment explaining what, because the latter isn't
> locally obvious.
True - we lex all known forms, and then only parse the current
platform's form. Will improve the comment.
>> } else if (!strcmp(token->str, "%ld")) {
>> return QOBJECT(qnum_from_int(va_arg(*ap, long)));
>> - } else if (!strcmp(token->str, "%lld") ||
>> - !strcmp(token->str, "%I64d")) {
>> + } else if (!strcmp(token->str, "%" PRId64)) {
>> + return QOBJECT(qnum_from_int(va_arg(*ap, int64_t)));
>> + } else if (!strcmp(token->str, "%lld")) {
>> return QOBJECT(qnum_from_int(va_arg(*ap, long long)));
>
> Let's do "ll" before PRId64.
Sure.
>> +++ b/tests/check-qjson.c
>> @@ -990,8 +990,10 @@ static void vararg_number(void)
>> QNum *qnum;
>> int value = 0x2342;
>> long long value_ll = 0x2342342343LL;
>> + uint64_t value_u = UINT64_C(0x2342342343);
>
> Is UINT64_C() really necessary here?
Not as long as none of the compilers we use complains about uint64_t x =
1ULL. I'll simplify, then we can uglify if a compiler complains.
>
> Call the variable value_u64?
Yes.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
