Re: [PATCH] Come up with json::integer_number and use it in GCOV.
On 9/18/19 2:04 AM, Martin Liška wrote: > PING^4 > > Just note that the author of the JSON implementation > in GCC is fine with the patch ;) OK if this is still pending :-) jeff
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
PING^4 Just note that the author of the JSON implementation in GCC is fine with the patch ;) Martin On 9/9/19 2:38 PM, Martin Liška wrote: > PING^3 > > On 8/30/19 10:55 AM, Martin Liška wrote: >> PING^2 >> >> On 8/26/19 2:34 PM, Martin Liška wrote: >>> PING^1 >>> >>> On 8/13/19 1:51 PM, Martin Liška wrote: On 8/2/19 2:40 PM, David Malcolm wrote: > Something that occurred to me reading the updated patch: maybe it would > make things easier to have utility member functions of json::object to > implicitly make the child, e.g.: > > void > json::object::set (const char *key, long v) > { >set (key, new json::integer_number (v)); > } > > so that all those calls can be just: > > obj->set ("line", exploc.line); > obj->set ("column", exploc.column); > > etc (assuming overloading is unambiguous). > > But that's probably orthogonal to this patch. Looks good to me. It's a candidate for a follow up patch. > > >> And I changed all occurrences of float_number with integer_number >> as you suggested. > Thanks. > >> I'm currently testing the updated patch. >> Martin > The updated patch looks good to me, but technically I'm not a reviewer > for these files. Sure, I hope @Jakub or @Richi can approve me that? Thanks, Martin > > Dave >>> >> >
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
PING^3 On 8/30/19 10:55 AM, Martin Liška wrote: > PING^2 > > On 8/26/19 2:34 PM, Martin Liška wrote: >> PING^1 >> >> On 8/13/19 1:51 PM, Martin Liška wrote: >>> On 8/2/19 2:40 PM, David Malcolm wrote: Something that occurred to me reading the updated patch: maybe it would make things easier to have utility member functions of json::object to implicitly make the child, e.g.: void json::object::set (const char *key, long v) { set (key, new json::integer_number (v)); } so that all those calls can be just: obj->set ("line", exploc.line); obj->set ("column", exploc.column); etc (assuming overloading is unambiguous). But that's probably orthogonal to this patch. >>> >>> Looks good to me. It's a candidate for a follow up patch. >>> > And I changed all occurrences of float_number with integer_number > as you suggested. Thanks. > I'm currently testing the updated patch. > Martin The updated patch looks good to me, but technically I'm not a reviewer for these files. >>> >>> Sure, I hope @Jakub or @Richi can approve me that? >>> Thanks, >>> Martin >>> Dave >>> >> >
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
PING^2 On 8/26/19 2:34 PM, Martin Liška wrote: > PING^1 > > On 8/13/19 1:51 PM, Martin Liška wrote: >> On 8/2/19 2:40 PM, David Malcolm wrote: >>> Something that occurred to me reading the updated patch: maybe it would >>> make things easier to have utility member functions of json::object to >>> implicitly make the child, e.g.: >>> >>> void >>> json::object::set (const char *key, long v) >>> { >>>set (key, new json::integer_number (v)); >>> } >>> >>> so that all those calls can be just: >>> >>> obj->set ("line", exploc.line); >>> obj->set ("column", exploc.column); >>> >>> etc (assuming overloading is unambiguous). >>> >>> But that's probably orthogonal to this patch. >> >> Looks good to me. It's a candidate for a follow up patch. >> >>> >>> And I changed all occurrences of float_number with integer_number as you suggested. >>> Thanks. >>> I'm currently testing the updated patch. Martin >>> The updated patch looks good to me, but technically I'm not a reviewer >>> for these files. >> >> Sure, I hope @Jakub or @Richi can approve me that? >> Thanks, >> Martin >> >>> >>> Dave >> >
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
PING^1 On 8/13/19 1:51 PM, Martin Liška wrote: > On 8/2/19 2:40 PM, David Malcolm wrote: >> Something that occurred to me reading the updated patch: maybe it would >> make things easier to have utility member functions of json::object to >> implicitly make the child, e.g.: >> >> void >> json::object::set (const char *key, long v) >> { >>set (key, new json::integer_number (v)); >> } >> >> so that all those calls can be just: >> >> obj->set ("line", exploc.line); >> obj->set ("column", exploc.column); >> >> etc (assuming overloading is unambiguous). >> >> But that's probably orthogonal to this patch. > > Looks good to me. It's a candidate for a follow up patch. > >> >> >>> And I changed all occurrences of float_number with integer_number >>> as you suggested. >> Thanks. >> >>> I'm currently testing the updated patch. >>> Martin >> The updated patch looks good to me, but technically I'm not a reviewer >> for these files. > > Sure, I hope @Jakub or @Richi can approve me that? > Thanks, > Martin > >> >> Dave >
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
On 8/2/19 2:40 PM, David Malcolm wrote: > Something that occurred to me reading the updated patch: maybe it would > make things easier to have utility member functions of json::object to > implicitly make the child, e.g.: > > void > json::object::set (const char *key, long v) > { >set (key, new json::integer_number (v)); > } > > so that all those calls can be just: > > obj->set ("line", exploc.line); > obj->set ("column", exploc.column); > > etc (assuming overloading is unambiguous). > > But that's probably orthogonal to this patch. Looks good to me. It's a candidate for a follow up patch. > > >> And I changed all occurrences of float_number with integer_number >> as you suggested. > Thanks. > >> I'm currently testing the updated patch. >> Martin > The updated patch looks good to me, but technically I'm not a reviewer > for these files. Sure, I hope @Jakub or @Richi can approve me that? Thanks, Martin > > Dave
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
On Fri, 2019-08-02 at 12:22 +0200, Martin Liška wrote: > On 7/31/19 3:13 PM, David Malcolm wrote: > > On Wed, 2019-07-31 at 10:42 +0200, Martin Liška wrote: > > > Hi. > > > > > > As seen here: > > > https://github.com/RPGillespie6/fastcov/pull/18/files/f184dd8b6fc > > > 14e0 > > > 75dfc0ea93de7be5c96298ddb#r308735088 > > > > > > GCOV uses json::number for branch count, line count and similar. > > > However, the json library > > > uses a double as an internal representation for numbers. That's > > > no > > > desirable in case > > > of GCOV and so that I would like to come up with integer_number > > > type. > > > David would you be fine with that? > > > > Thanks for the patch; overall I'm broadly in favor of the idea, but > > I > > think the patch needs a little work. > > > > The JSON standard has a definition for numbers that allows for both > > integers and floats, and says this about interoperability: > > > > https://tools.ietf.org/html/rfc7159#section-6 > > https://tools.ietf.org/html/rfc8259#section-6 > > > > "This specification allows implementations to set limits on the > > range > >and precision of numbers accepted. Since software that > > implements > >IEEE 754 binary64 (double precision) numbers [IEEE754] is > > generally > >available and widely used, good interoperability can be achieved > > by > >implementations that expect no more precision or range than > > these > >provide, in the sense that implementations will approximate JSON > >numbers within the expected precision." > > > > Hence I chose "double" as the representation. But, yeah, it's a > > pain > > when dealing with large integers. > > > > [see also "Parsing JSON is a Minefield" > > http://seriot.ch/parsing_json.php#22 ] > > > > Looking at your patch, you convert some places to integer_number, > > and > > some to float_number. > > > > It looks to me like you converted the gcov places you were > > concerned > > about to integer, and kept the "status quo" as floats for the other > > ones. But in all of the places I can see, I think integers make > > more > > sense than floats. > > Yep, but you are right that all other places also needed integer_type > :) > > > > > I think we want to preserve the capability to emit floating point > > json > > values, but I suspect we want to use integer values everywhere > > we're > > currently emitting json; it's probably worth going through them > > line by > > line... > > I'm fine with preservation of the type. > > > > > > diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic- > > > format-json.cc > > > index 53c3b630b1c..2802da8a0a6 100644 > > > --- a/gcc/diagnostic-format-json.cc > > > +++ b/gcc/diagnostic-format-json.cc > > > @@ -48,8 +48,8 @@ json_from_expanded_location (location_t loc) > > >json::object *result = new json::object (); > > >if (exploc.file) > > > result->set ("file", new json::string (exploc.file)); > > > - result->set ("line", new json::number (exploc.line)); > > > - result->set ("column", new json::number (exploc.column)); > > > + result->set ("line", new json::float_number (exploc.line)); > > > + result->set ("column", new json::float_number > > > (exploc.column)); > > > > These should be integers. > > > > > > [...snip gcov hunks...] > > > > > diff --git a/gcc/json.cc b/gcc/json.cc > > > index 512e2e513b9..bec6fc53cc8 100644 > > > --- a/gcc/json.cc > > > +++ b/gcc/json.cc > > > @@ -154,18 +154,31 @@ array::append (value *v) > > >m_elements.safe_push (v); > > > } > > > > > > -/* class json::number, a subclass of json::value, wrapping a > > > double. */ > > > +/* class json::float_number, a subclass of json::value, wrapping > > > a double. */ > > > > Would it make more sense to call this "double_number"? (am not > > sure) > > I would prefer to stay with json::float_number. Fair enough. > > > > > > > -/* Implementation of json::value::print for json::number. */ > > > +/* Implementation of json::value::print for > > > json::float_number. */ > > > > > > void > > > -number::print (pretty_printer *pp) const > > > +float_number::print (pretty_printer *pp) const > > > { > > >char tmp[1024]; > > >snprintf (tmp, sizeof (tmp), "%g", m_value); > > >pp_string (pp, tmp); > > > } > > > > > > +/* class json::integer_number, a subclass of json::value, > > > wrapping a long. */ > > > > Likewise, would it make more sense to call this "long"? > > > > An idea I had reading your patch: would a template be appropriate > > here, > > something like: > > > > template > > class number : public value > > { > > enum kind get_kind () const FINAL OVERRIDE; > > T get () const { return m_value; } > > /* etc */ > > > > T m_value; > > }; > > > > with specializations for "double" and "long"? > > (hence json::number json::number, and enum values in > > the > > json_kind discriminator>). > > > > I think that a template might be overthinking things, though; > > the approach in your patch has
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
On 7/31/19 3:13 PM, David Malcolm wrote: > On Wed, 2019-07-31 at 10:42 +0200, Martin Liška wrote: >> Hi. >> >> As seen here: >> https://github.com/RPGillespie6/fastcov/pull/18/files/f184dd8b6fc14e0 >> 75dfc0ea93de7be5c96298ddb#r308735088 >> >> GCOV uses json::number for branch count, line count and similar. >> However, the json library >> uses a double as an internal representation for numbers. That's no >> desirable in case >> of GCOV and so that I would like to come up with integer_number type. >> David would you be fine with that? > > Thanks for the patch; overall I'm broadly in favor of the idea, but I > think the patch needs a little work. > > The JSON standard has a definition for numbers that allows for both > integers and floats, and says this about interoperability: > > https://tools.ietf.org/html/rfc7159#section-6 > https://tools.ietf.org/html/rfc8259#section-6 > > "This specification allows implementations to set limits on the range >and precision of numbers accepted. Since software that implements >IEEE 754 binary64 (double precision) numbers [IEEE754] is generally >available and widely used, good interoperability can be achieved by >implementations that expect no more precision or range than these >provide, in the sense that implementations will approximate JSON >numbers within the expected precision." > > Hence I chose "double" as the representation. But, yeah, it's a pain > when dealing with large integers. > > [see also "Parsing JSON is a Minefield" > http://seriot.ch/parsing_json.php#22 ] > > Looking at your patch, you convert some places to integer_number, and > some to float_number. > > It looks to me like you converted the gcov places you were concerned > about to integer, and kept the "status quo" as floats for the other > ones. But in all of the places I can see, I think integers make more > sense than floats. Yep, but you are right that all other places also needed integer_type :) > > I think we want to preserve the capability to emit floating point json > values, but I suspect we want to use integer values everywhere we're > currently emitting json; it's probably worth going through them line by > line... I'm fine with preservation of the type. > >> diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc >> index 53c3b630b1c..2802da8a0a6 100644 >> --- a/gcc/diagnostic-format-json.cc >> +++ b/gcc/diagnostic-format-json.cc >> @@ -48,8 +48,8 @@ json_from_expanded_location (location_t loc) >>json::object *result = new json::object (); >>if (exploc.file) >> result->set ("file", new json::string (exploc.file)); >> - result->set ("line", new json::number (exploc.line)); >> - result->set ("column", new json::number (exploc.column)); >> + result->set ("line", new json::float_number (exploc.line)); >> + result->set ("column", new json::float_number (exploc.column)); > > These should be integers. > > > [...snip gcov hunks...] > >> diff --git a/gcc/json.cc b/gcc/json.cc >> index 512e2e513b9..bec6fc53cc8 100644 >> --- a/gcc/json.cc >> +++ b/gcc/json.cc >> @@ -154,18 +154,31 @@ array::append (value *v) >>m_elements.safe_push (v); >> } >> >> -/* class json::number, a subclass of json::value, wrapping a double. */ >> +/* class json::float_number, a subclass of json::value, wrapping a double. >> */ > > Would it make more sense to call this "double_number"? (am not sure) I would prefer to stay with json::float_number. > > >> -/* Implementation of json::value::print for json::number. */ >> +/* Implementation of json::value::print for json::float_number. */ >> >> void >> -number::print (pretty_printer *pp) const >> +float_number::print (pretty_printer *pp) const >> { >>char tmp[1024]; >>snprintf (tmp, sizeof (tmp), "%g", m_value); >>pp_string (pp, tmp); >> } >> >> +/* class json::integer_number, a subclass of json::value, wrapping a long. >> */ > > Likewise, would it make more sense to call this "long"? > > An idea I had reading your patch: would a template be appropriate here, > something like: > > template > class number : public value > { > enum kind get_kind () const FINAL OVERRIDE; > T get () const { return m_value; } > /* etc */ > > T m_value; > }; > > with specializations for "double" and "long"? > (hence json::number json::number, and enum values in the > json_kind discriminator>). > > I think that a template might be overthinking things, though; > the approach in your patch has the virtue of simplicity. > > Is "long" always wide enough for all the integer values we might want > to express on the host? Well, I would not over engineer it :) > > [...snip...] > >> -/* Subclass of value for numbers. */ >> +/* Subclass of value for floats. */ > > I'd write "floating-point numbers" here (given that JSON standard > talks about "numbers". Changed. > > [...] > >> +/* Subclass of value for integers. */ > > Likewise "integer-valued numbers" here, or
Re: [PATCH] Come up with json::integer_number and use it in GCOV.
On Wed, 2019-07-31 at 10:42 +0200, Martin Liška wrote: > Hi. > > As seen here: > https://github.com/RPGillespie6/fastcov/pull/18/files/f184dd8b6fc14e0 > 75dfc0ea93de7be5c96298ddb#r308735088 > > GCOV uses json::number for branch count, line count and similar. > However, the json library > uses a double as an internal representation for numbers. That's no > desirable in case > of GCOV and so that I would like to come up with integer_number type. > David would you be fine with that? Thanks for the patch; overall I'm broadly in favor of the idea, but I think the patch needs a little work. The JSON standard has a definition for numbers that allows for both integers and floats, and says this about interoperability: https://tools.ietf.org/html/rfc7159#section-6 https://tools.ietf.org/html/rfc8259#section-6 "This specification allows implementations to set limits on the range and precision of numbers accepted. Since software that implements IEEE 754 binary64 (double precision) numbers [IEEE754] is generally available and widely used, good interoperability can be achieved by implementations that expect no more precision or range than these provide, in the sense that implementations will approximate JSON numbers within the expected precision." Hence I chose "double" as the representation. But, yeah, it's a pain when dealing with large integers. [see also "Parsing JSON is a Minefield" http://seriot.ch/parsing_json.php#22 ] Looking at your patch, you convert some places to integer_number, and some to float_number. It looks to me like you converted the gcov places you were concerned about to integer, and kept the "status quo" as floats for the other ones. But in all of the places I can see, I think integers make more sense than floats. I think we want to preserve the capability to emit floating point json values, but I suspect we want to use integer values everywhere we're currently emitting json; it's probably worth going through them line by line... > diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc > index 53c3b630b1c..2802da8a0a6 100644 > --- a/gcc/diagnostic-format-json.cc > +++ b/gcc/diagnostic-format-json.cc > @@ -48,8 +48,8 @@ json_from_expanded_location (location_t loc) >json::object *result = new json::object (); >if (exploc.file) > result->set ("file", new json::string (exploc.file)); > - result->set ("line", new json::number (exploc.line)); > - result->set ("column", new json::number (exploc.column)); > + result->set ("line", new json::float_number (exploc.line)); > + result->set ("column", new json::float_number (exploc.column)); These should be integers. [...snip gcov hunks...] > diff --git a/gcc/json.cc b/gcc/json.cc > index 512e2e513b9..bec6fc53cc8 100644 > --- a/gcc/json.cc > +++ b/gcc/json.cc > @@ -154,18 +154,31 @@ array::append (value *v) >m_elements.safe_push (v); > } > > -/* class json::number, a subclass of json::value, wrapping a double. */ > +/* class json::float_number, a subclass of json::value, wrapping a double. > */ Would it make more sense to call this "double_number"? (am not sure) > -/* Implementation of json::value::print for json::number. */ > +/* Implementation of json::value::print for json::float_number. */ > > void > -number::print (pretty_printer *pp) const > +float_number::print (pretty_printer *pp) const > { >char tmp[1024]; >snprintf (tmp, sizeof (tmp), "%g", m_value); >pp_string (pp, tmp); > } > > +/* class json::integer_number, a subclass of json::value, wrapping a long. > */ Likewise, would it make more sense to call this "long"? An idea I had reading your patch: would a template be appropriate here, something like: template class number : public value { enum kind get_kind () const FINAL OVERRIDE; T get () const { return m_value; } /* etc */ T m_value; }; with specializations for "double" and "long"? (hence json::number json::number, and enum values in the json_kind discriminator>). I think that a template might be overthinking things, though; the approach in your patch has the virtue of simplicity. Is "long" always wide enough for all the integer values we might want to express on the host? [...snip...] > -/* Subclass of value for numbers. */ > +/* Subclass of value for floats. */ I'd write "floating-point numbers" here (given that JSON standard talks about "numbers". [...] > +/* Subclass of value for integers. */ Likewise "integer-valued numbers" here, or somesuch. [...] > diff --git a/gcc/optinfo-emit-json.cc b/gcc/optinfo-emit-json.cc > index 1cfcdfe8948..87cc940133a 100644 > --- a/gcc/optinfo-emit-json.cc > +++ b/gcc/optinfo-emit-json.cc > @@ -181,7 +181,7 @@ optrecord_json_writer::impl_location_to_json > (dump_impl_location_t loc) > { >json::object *obj = new json::object (); >obj->set ("file", new json::string (loc.m_file)); > - obj->set ("line", new json::number (loc.m_line)); > + obj->set ("line", new
[PATCH] Come up with json::integer_number and use it in GCOV.
Hi. As seen here: https://github.com/RPGillespie6/fastcov/pull/18/files/f184dd8b6fc14e075dfc0ea93de7be5c96298ddb#r308735088 GCOV uses json::number for branch count, line count and similar. However, the json library uses a double as an internal representation for numbers. That's no desirable in case of GCOV and so that I would like to come up with integer_number type. David would you be fine with that? Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: 2019-07-31 Martin Liska * diagnostic-format-json.cc (json_from_expanded_location): Use json::float_number. * gcov.c (output_intermediate_json_line): Use new json::integer_number. (output_json_intermediate_file): Likewise. * json.cc (number::print): Move to ... (float_number::print): ... this. (integer_number::print): New. (test_writing_numbers): Move to ... (test_writing_float_numbers): ... this. (test_writing_integer_numbers): New. (json_cc_tests): Register test_writing_integer_numbers. * json.h (class value): Add forward declaration for float_number and integer_number. (enum kind): Add JSON_INTEGER and JSON_FLOAT. (class number): Move to ... (class float_number): ... this. (class integer_number): New. * optinfo-emit-json.cc (optrecord_json_writer::impl_location_to_json): Use json::float_number. (optrecord_json_writer::location_to_json): Likewise. (optrecord_json_writer::profile_count_to_json): Likewise. (optrecord_json_writer::pass_to_json): Likewise. --- gcc/diagnostic-format-json.cc | 4 ++-- gcc/gcov.c| 23 +++- gcc/json.cc | 41 --- gcc/json.h| 35 -- gcc/optinfo-emit-json.cc | 10 - 5 files changed, 81 insertions(+), 32 deletions(-) diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc index 53c3b630b1c..2802da8a0a6 100644 --- a/gcc/diagnostic-format-json.cc +++ b/gcc/diagnostic-format-json.cc @@ -48,8 +48,8 @@ json_from_expanded_location (location_t loc) json::object *result = new json::object (); if (exploc.file) result->set ("file", new json::string (exploc.file)); - result->set ("line", new json::number (exploc.line)); - result->set ("column", new json::number (exploc.column)); + result->set ("line", new json::float_number (exploc.line)); + result->set ("column", new json::float_number (exploc.column)); return result; } diff --git a/gcc/gcov.c b/gcc/gcov.c index c65b7153765..ef006d125a2 100644 --- a/gcc/gcov.c +++ b/gcc/gcov.c @@ -1061,10 +1061,10 @@ output_intermediate_json_line (json::array *object, return; json::object *lineo = new json::object (); - lineo->set ("line_number", new json::number (line_num)); + lineo->set ("line_number", new json::integer_number (line_num)); if (function_name != NULL) lineo->set ("function_name", new json::string (function_name)); - lineo->set ("count", new json::number (line->count)); + lineo->set ("count", new json::integer_number (line->count)); lineo->set ("unexecuted_block", new json::literal (line->has_unexecuted_block)); @@ -1079,7 +1079,7 @@ output_intermediate_json_line (json::array *object, if (!(*it)->is_unconditional && !(*it)->is_call_non_return) { json::object *branch = new json::object (); - branch->set ("count", new json::number ((*it)->count)); + branch->set ("count", new json::integer_number ((*it)->count)); branch->set ("throw", new json::literal ((*it)->is_throw)); branch->set ("fallthrough", new json::literal ((*it)->fall_through)); @@ -1138,16 +1138,19 @@ output_json_intermediate_file (json::array *json_files, source_info *src) function->set ("name", new json::string ((*it)->m_name)); function->set ("demangled_name", new json::string ((*it)->get_demangled_name ())); - function->set ("start_line", new json::number ((*it)->start_line)); - function->set ("start_column", new json::number ((*it)->start_column)); - function->set ("end_line", new json::number ((*it)->end_line)); - function->set ("end_column", new json::number ((*it)->end_column)); + function->set ("start_line", + new json::integer_number ((*it)->start_line)); + function->set ("start_column", + new json::integer_number ((*it)->start_column)); + function->set ("end_line", new json::integer_number ((*it)->end_line)); + function->set ("end_column", + new json::integer_number ((*it)->end_column)); function->set ("blocks", - new json::number ((*it)->get_block_count ())); + new json::integer_number ((*it)->get_block_count ())); function->set ("blocks_executed", - new json::number ((*it)->blocks_executed));