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 <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 :)
>
> [...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.
>
>>
And I changed all occurrences of float_number with integer_number
as you suggested.
I'm currently testing the updated patch.
Martin
>From 0863b4e326838fdd16c423bd0b8f59f1dfd7b0f0 Mon Sep 17 00:00:00 2001
From: Martin Liska <[email protected]>
Date: Wed, 31 Jul 2019 09:51:28 +0200
Subject: [PATCH] Come up with json::integer_number and use it in GCOV.
gcc/ChangeLog:
2019-07-31 Martin Liska <[email protected]>
* diagnostic-format-json.cc (json_from_expanded_location):
Use json::integer_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::integer_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..ae812174ad7 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::integer_number (exploc.line));
+ result->set ("column", new json::integer_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));
+ new json::integer_number ((*it)->blocks_executed));
function->set ("execution_count",
- new json::number ((*it)->blocks[0].count));
+ new json::integer_number ((*it)->blocks[0].count));
functions->append (function);
}
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. */
-/* 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. */
+
+/* Implementation of json::value::print for json::integer_number. */
+
+void
+integer_number::print (pretty_printer *pp) const
+{
+ char tmp[1024];
+ snprintf (tmp, sizeof (tmp), "%ld", m_value);
+ pp_string (pp, tmp);
+}
+
+
/* class json::string, a subclass of json::value. */
/* json::string's ctor. */
@@ -297,11 +310,22 @@ test_writing_arrays ()
/* Verify that JSON numbers are written correctly. */
static void
-test_writing_numbers ()
+test_writing_float_numbers ()
+{
+ assert_print_eq (float_number (0), "0");
+ assert_print_eq (float_number (42), "42");
+ assert_print_eq (float_number (-100), "-100");
+ assert_print_eq (float_number (123456789), "1.23457e+08");
+}
+
+static void
+test_writing_integer_numbers ()
{
- assert_print_eq (number (0), "0");
- assert_print_eq (number (42), "42");
- assert_print_eq (number (-100), "-100");
+ assert_print_eq (integer_number (0), "0");
+ assert_print_eq (integer_number (42), "42");
+ assert_print_eq (integer_number (-100), "-100");
+ assert_print_eq (integer_number (123456789), "123456789");
+ assert_print_eq (integer_number (-123456789), "-123456789");
}
/* Verify that JSON strings are written correctly. */
@@ -337,7 +361,8 @@ json_cc_tests ()
test_object_get ();
test_writing_objects ();
test_writing_arrays ();
- test_writing_numbers ();
+ test_writing_float_numbers ();
+ test_writing_integer_numbers ();
test_writing_strings ();
test_writing_literals ();
}
diff --git a/gcc/json.h b/gcc/json.h
index d8a690ede5c..316bc8b254c 100644
--- a/gcc/json.h
+++ b/gcc/json.h
@@ -39,7 +39,8 @@ namespace json
class value;
class object;
class array;
- class number;
+ class float_number;
+ class integer_number;
class string;
class literal;
@@ -53,8 +54,11 @@ enum kind
/* class json::array. */
JSON_ARRAY,
- /* class json::number. */
- JSON_NUMBER,
+ /* class json::integer_number. */
+ JSON_INTEGER,
+
+ /* class json::float_number. */
+ JSON_FLOAT,
/* class json::string. */
JSON_STRING,
@@ -114,14 +118,14 @@ class array : public value
auto_vec<value *> m_elements;
};
-/* Subclass of value for numbers. */
+/* Subclass of value for floating-point numbers. */
-class number : public value
+class float_number : public value
{
public:
- number (double value) : m_value (value) {}
+ float_number (double value) : m_value (value) {}
- enum kind get_kind () const FINAL OVERRIDE { return JSON_NUMBER; }
+ enum kind get_kind () const FINAL OVERRIDE { return JSON_FLOAT; }
void print (pretty_printer *pp) const FINAL OVERRIDE;
double get () const { return m_value; }
@@ -130,6 +134,23 @@ class number : public value
double m_value;
};
+/* Subclass of value for integer-valued numbers. */
+
+class integer_number : public value
+{
+ public:
+ integer_number (long value) : m_value (value) {}
+
+ enum kind get_kind () const FINAL OVERRIDE { return JSON_INTEGER; }
+ void print (pretty_printer *pp) const FINAL OVERRIDE;
+
+ long get () const { return m_value; }
+
+ private:
+ long m_value;
+};
+
+
/* Subclass of value for strings. */
class string : public value
diff --git a/gcc/optinfo-emit-json.cc b/gcc/optinfo-emit-json.cc
index 1cfcdfe8948..1ca4f148d15 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::integer_number (loc.m_line));
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::integer_number (exploc.line));
+ obj->set ("column", new json::integer_number (exploc.column));
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::integer_number (count.to_gcov_type ()));
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::integer_number (pass->static_pass_number));
return obj;
}
--
2.22.0
