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

[David Malcolm <dmalcolm@redhat.com>]

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.

>