Re: [PATCH] Come up with json::integer_number and use it in GCOV.

[David Malcolm <dmalcolm at redhat dot com>]

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