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 <typename T> 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<double> json::number<long>, 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 json::float_number (loc.m_line)); integer here, not float. > if (loc.m_function) > obj->set ("function", new json::string (loc.m_function)); > return obj; > @@ -196,8 +196,8 @@ optrecord_json_writer::location_to_json (location_t loc) > expanded_location exploc = expand_location (loc); > json::object *obj = new json::object (); > obj->set ("file", new json::string (exploc.file)); > - obj->set ("line", new json::number (exploc.line)); > - obj->set ("column", new json::number (exploc.column)); > + obj->set ("line", new json::float_number (exploc.line)); > + obj->set ("column", new json::float_number (exploc.column)); Likewise, integers here. > return obj; > } > > @@ -207,7 +207,7 @@ json::object * > optrecord_json_writer::profile_count_to_json (profile_count count) > { > json::object *obj = new json::object (); > - obj->set ("value", new json::number (count.to_gcov_type ())); > + obj->set ("value", new json::float_number (count.to_gcov_type ())); Presumably this should be an integer also. > obj->set ("quality", > new json::string (profile_quality_as_string (count.quality ()))); > return obj; > @@ -262,7 +262,7 @@ optrecord_json_writer::pass_to_json (opt_pass *pass) > && (pass->optinfo_flags & optgroup->value)) > optgroups->append (new json::string (optgroup->name)); > } > - obj->set ("num", new json::number (pass->static_pass_number)); > + obj->set ("num", new json::float_number (pass->static_pass_number)); Likewise. >
