Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Jeff Hostetler]

On 3/26/2018 11:26 PM, Ramsay Jones wrote:

On 26/03/18 18:04, Junio C Hamano wrote:

Ramsay Jones  writes:

[...]

I must confess to not having given any thought to the wider
implications of the code. I don't really know what this code
is going to be used for. [Although I did shudder when I read
some mention of a 'universal interchange format' - I still
have nightmares about XML :-D ]

[...]

My current goals are to add telemetry in a friendly way and
have events written in JSON to some audit destination.
Something like:

{ "argv":["./git","status"],
  "pid":84941,
  "exit-code":0,
  "elapsed-time":0.011121,
  "version":"2.16.2.5.g71445db.dirty",
  ... }

Later, we could add a JSON formatter to a command like "status"
and then do things like:

$ git status --json | python '... json.load ...'

and eliminate the need to write custom parsers for normal
or porcelain formats.  There are other commands that could
be similarly adapted and save callers a lot of screen-scraping
code.  But that is later.

Thanks,
Jeff

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Ramsay Jones]

On 26/03/18 18:04, Junio C Hamano wrote:
> Ramsay Jones  writes:
> 
 @@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const 
 char *key, uint64_t value)
maybe_add_comma(jw);
  
append_quoted_string(>json, key);
 -  strbuf_addf(>json, ":%"PRIuMAX, value);
 +  strbuf_addf(>json, ":%"PRIu64, value);
>>>
>>> In this code-base, that would normally be written as:
>>>
>>> strbuf_addf(>json, ":%"PRIuMAX, (uintmax_t) value);
>>
>> heh, I should learn not to reply in a hurry, just before
>> going out ...
>>
>> I had not noticed that 'value' was declared with an 'sized type'
>> of uint64_t, so using PRIu64 should be fine.
> 
> But why is this codepath using a sized type in the first place?  It
> is not like it wants to read/write a fixed binary file format---it
> just wants to use an integer type that is wide enough to handle any
> inttype the platform uses, for which uintmax_t would be a more
> appropriate type, no?

I must confess to not having given any thought to the wider
implications of the code. I don't really know what this code
is going to be used for. [Although I did shudder when I read
some mention of a 'universal interchange format' - I still
have nightmares about XML :-D ]

ATB,
Ramsay Jones

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Jeff Hostetler]

On 3/26/2018 1:04 PM, Junio C Hamano wrote:

Ramsay Jones  writes:


@@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const char 
*key, uint64_t value)
maybe_add_comma(jw);
  
  	append_quoted_string(>json, key);

-   strbuf_addf(>json, ":%"PRIuMAX, value);
+   strbuf_addf(>json, ":%"PRIu64, value);


In this code-base, that would normally be written as:

strbuf_addf(>json, ":%"PRIuMAX, (uintmax_t) value);


heh, I should learn not to reply in a hurry, just before
going out ...

I had not noticed that 'value' was declared with an 'sized type'
of uint64_t, so using PRIu64 should be fine.


But why is this codepath using a sized type in the first place?  It
is not like it wants to read/write a fixed binary file format---it
just wants to use an integer type that is wide enough to handle any
inttype the platform uses, for which uintmax_t would be a more
appropriate type, no?



[Somehow the conversation forked and this compiler warning
appeared in both the json-writer and the rebase-interactive
threads.  I'm copying here the response that I already made
on the latter.]


I defined that routine to take a uint64_t because I wanted to
pass a nanosecond value received from getnanotime() and that's
what it returns.

My preference would be to change the PRIuMAX to PRIu64, but there
aren't any other references in the code to that symbol and I didn't
want to start a new trend here.

I am concerned that the above compiler error message says that uintmax_t
is defined as an "unsigned long" (which is defined as *at least* 32 bits,
but not necessarily 64.  But a uint64_t is defined as a "unsigned long long"
and guaranteed as a 64 bit value.

So while I'm not really worried about 128 bit integers right now, I'm
more concerned about 32 bit compilers truncating that value without any
warnings.

Jeff

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Junio C Hamano]

Ramsay Jones  writes:

>>> @@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const 
>>> char *key, uint64_t value)
>>> maybe_add_comma(jw);
>>>  
>>> append_quoted_string(>json, key);
>>> -   strbuf_addf(>json, ":%"PRIuMAX, value);
>>> +   strbuf_addf(>json, ":%"PRIu64, value);
>> 
>> In this code-base, that would normally be written as:
>> 
>>  strbuf_addf(>json, ":%"PRIuMAX, (uintmax_t) value);
>
> heh, I should learn not to reply in a hurry, just before
> going out ...
>
> I had not noticed that 'value' was declared with an 'sized type'
> of uint64_t, so using PRIu64 should be fine.

But why is this codepath using a sized type in the first place?  It
is not like it wants to read/write a fixed binary file format---it
just wants to use an integer type that is wide enough to handle any
inttype the platform uses, for which uintmax_t would be a more
appropriate type, no?

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Ramsay Jones]

On 24/03/18 15:14, Ramsay Jones wrote:
> 
> 
> On 24/03/18 05:37, Wink Saville wrote:
>> In routines jw_object_uint64 and jw_object_double strbuf_addf is
>> invoked with strbuf_addf(>json, ":%"PRIuMAX, value) where value
>> is a uint64_t. This causes a compile error on OSX.
>>
>> The correct format specifier is PRIu64 instead of PRIuMax.
>>
>> Signed-off-by: Wink Saville 
>> ---
>>  json-writer.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/json-writer.c b/json-writer.c
>> index 89a6abb57..04045448a 100644
>> --- a/json-writer.c
>> +++ b/json-writer.c
>> @@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const char 
>> *key, uint64_t value)
>>  maybe_add_comma(jw);
>>  
>>  append_quoted_string(>json, key);
>> -strbuf_addf(>json, ":%"PRIuMAX, value);
>> +strbuf_addf(>json, ":%"PRIu64, value);
> 
> In this code-base, that would normally be written as:
> 
>   strbuf_addf(>json, ":%"PRIuMAX, (uintmax_t) value);

heh, I should learn not to reply in a hurry, just before
going out ...

I had not noticed that 'value' was declared with an 'sized type'
of uint64_t, so using PRIu64 should be fine. Well, except that
you may have to add a 'fallback' definition of PRIu64 to one of
the 'compat/mingw.h', 'compat/msvc.h' or 'git-compat-util.h'
header files. (see e.g. PRId64 at compat/mingw.h:429).

[About a decade ago, I heard microsoft were implementing C99
'real soon now' ;-) ]

ATB,
Ramsay Jones

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Ramsay Jones]

On 24/03/18 05:37, Wink Saville wrote:
> In routines jw_object_uint64 and jw_object_double strbuf_addf is
> invoked with strbuf_addf(>json, ":%"PRIuMAX, value) where value
> is a uint64_t. This causes a compile error on OSX.
> 
> The correct format specifier is PRIu64 instead of PRIuMax.
> 
> Signed-off-by: Wink Saville 
> ---
>  json-writer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/json-writer.c b/json-writer.c
> index 89a6abb57..04045448a 100644
> --- a/json-writer.c
> +++ b/json-writer.c
> @@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const char 
> *key, uint64_t value)
>   maybe_add_comma(jw);
>  
>   append_quoted_string(>json, key);
> - strbuf_addf(>json, ":%"PRIuMAX, value);
> + strbuf_addf(>json, ":%"PRIu64, value);

In this code-base, that would normally be written as:

strbuf_addf(>json, ":%"PRIuMAX, (uintmax_t) value);

ATB,
Ramsay Jones

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Jeff Hostetler]

On 3/24/2018 1:37 AM, Wink Saville wrote:

In routines jw_object_uint64 and jw_object_double strbuf_addf is
invoked with strbuf_addf(>json, ":%"PRIuMAX, value) where value
is a uint64_t. This causes a compile error on OSX.

The correct format specifier is PRIu64 instead of PRIuMax.

Signed-off-by: Wink Saville 


That's odd.

A grep on the Git source tree did not find a "PRIu64" symbol.
Searching public-inbox only found one message [1] talking about it
(other than the ones associated with your messages here).

I have to wonder if that is defined in a OSX header file and
you're getting it from there [2].  (I don't have a MAC in front of
me, so I can't verify what's in that header.)  But [2] defines
PRIuMAX as PRIu64, so we shouldn't need to make that change in
json-writer -- unless something is getting lost in the #ifdefs.

Could you double check this in the header files on your system?
Any chance you are doing a 32-bit build?

Thanks
Jeff

[1] 
https://public-inbox.org/git/mwhpr21mb0478181ae0b64901da2c07cdf4...@mwhpr21mb0478.namprd21.prod.outlook.com/raw


[2] https://opensource.apple.com/source/gcc/gcc-926/inttypes.h.auto.html



---
  json-writer.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/json-writer.c b/json-writer.c
index 89a6abb57..04045448a 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const char 
*key, uint64_t value)
maybe_add_comma(jw);
  
  	append_quoted_string(>json, key);

-   strbuf_addf(>json, ":%"PRIuMAX, value);
+   strbuf_addf(>json, ":%"PRIu64, value);
  }
  
  void jw_object_double(struct json_writer *jw, const char *fmt,

@@ -225,7 +225,7 @@ void jw_array_uint64(struct json_writer *jw, uint64_t value)
assert_in_array(jw);
maybe_add_comma(jw);
  
-	strbuf_addf(>json, "%"PRIuMAX, value);

+   strbuf_addf(>json, "%"PRIu64, value);
  }
  
  void jw_array_double(struct json_writer *jw, const char *fmt, double value)

[RFC PATCH 1/1] json-writer: incorrect format specifier

[Wink Saville]

In routines jw_object_uint64 and jw_object_double strbuf_addf is
invoked with strbuf_addf(>json, ":%"PRIuMAX, value) where value
is a uint64_t. This causes a compile error on OSX.

The correct format specifier is PRIu64 instead of PRIuMax.

Signed-off-by: Wink Saville 
---
 json-writer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/json-writer.c b/json-writer.c
index 89a6abb57..04045448a 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -120,7 +120,7 @@ void jw_object_uint64(struct json_writer *jw, const char 
*key, uint64_t value)
maybe_add_comma(jw);
 
append_quoted_string(>json, key);
-   strbuf_addf(>json, ":%"PRIuMAX, value);
+   strbuf_addf(>json, ":%"PRIu64, value);
 }
 
 void jw_object_double(struct json_writer *jw, const char *fmt,
@@ -225,7 +225,7 @@ void jw_array_uint64(struct json_writer *jw, uint64_t value)
assert_in_array(jw);
maybe_add_comma(jw);
 
-   strbuf_addf(>json, "%"PRIuMAX, value);
+   strbuf_addf(>json, "%"PRIu64, value);
 }
 
 void jw_array_double(struct json_writer *jw, const char *fmt, double value)
-- 
2.16.2