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 <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?
>
> Well, I would not over engineer it :)
Likewise, fair enough.
> >
> > [...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 somesuch.
>
> Changed.
>
> >
> > [...]
> >
> > > 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.
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.
> 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.
Dave
