Fix issue#1719 #2044

dota17 · 2020-04-16T03:04:19Z

Pull request checklist

Read the Contribution Guidelines for detailed information.

Changes are described in the pull request, or an existing issue is referenced.
The test suite compiles and runs without error.
Code coverage is 100%. Test cases can be added by editing the test suite.
The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Describe

As discussed in issue #1719, I made this PR base on @rvjr 's patch, and also refer to the author's suggestions in this comment

rvjr · 2020-04-16T09:47:07Z

Is there any reason for the 'use_type' parameter? Or at least I would make its default value 'true'. I see no use case why we should serialize something as double instead of float when the actual value is identical.

nlohmann

Could you please check the errors in the Travis build?

nlohmann · 2020-05-02T12:37:44Z

@dota17 I checked the errors on Travis and it seems the use_type introduces this issue. I think my proposal to add this parameter does not make sense - we also silently use the smallest possible integer types and it would be odd not to treat floating-point numbers the same.

Could you please revert and remove the use_type parameter, and maybe also rebase to the latest develop branch?

dota17 · 2020-05-06T11:40:53Z

@nlohmann The commit has been modified according to your suggestion.
The Travis failed becasue E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?, but I think it could be ok if you can restart build job.

nlohmann

I think there was a misunderstanding. The PR now only adds a different encoding for NaN, +Inf, and -Inf. The patch from @rvjr also added an encoding to floats where possible. Can you re-add that - I only meant to remove the boolean parameter to switch on/off that encoding to floats as I think doing so is a good default.

Sorry for the confusion.

nlohmann

It seems that there are other CBOR tests failing.

include/nlohmann/detail/output/binary_writer.hpp

dota17 · 2020-05-08T09:37:27Z

It seems that there are other CBOR tests failing.

@nlohmann These CBOR tests failed:
0.0, -0.0, 0e+1, 0e1, 123.456e-789, 20e1, 1E+2, 1e+2, 123e-10000000
but the other CBOR tests passed. the problem is in the last piece of code, which comes from the original patch. Could you please check how to fix this?

nlohmann · 2020-05-12T10:55:56Z

It seems that there are other CBOR tests failing.

@nlohmann These CBOR tests failed:
0.0, -0.0, 0e+1, 0e1, 123.456e-789, 20e1, 1E+2, 1e+2, 123e-10000000
but the other CBOR tests passed. the problem is in the last piece of code, which comes from the original patch. Could you please check how to fix this?

I checked locally with some debug output:

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/json_roundtrip/roundtrip20.json
          ~/json/cmake-build-debug/json_test_data/json_roundtrip/roundtrip20.json: output to output adapters
          j1 := [0.0]
          stringify(packed) := 129 251 0 0 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/json_roundtrip/roundtrip20.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 0 0 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/json_roundtrip/roundtrip21.json
          ~/json/cmake-build-debug/json_test_data/json_roundtrip/roundtrip21.json: output to output adapters
          j1 := [-0.0]
          stringify(packed) := 129 251 128 0 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/json_roundtrip/roundtrip21.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 128 0 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_0e+1.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_0e+1.json: output to output adapters
          j1 := [0.0]
          stringify(packed) := 129 251 0 0 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_0e+1.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 0 0 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_0e1.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_0e1.json: output to output adapters
          j1 := [0.0]
          stringify(packed) := 129 251 0 0 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_0e1.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 0 0 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_double_huge_neg_exp.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_double_huge_neg_exp.json: output to output adapters
          j1 := [0.0]
          stringify(packed) := 129 251 0 0 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_double_huge_neg_exp.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 0 0 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_int_with_exp.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_int_with_exp.json: output to output adapters
          j1 := [200.0]
          stringify(packed) := 129 251 64 105 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_int_with_exp.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 67 72 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_capital_e_pos_exp.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_capital_e_pos_exp.json: output to output adapters
          j1 := [100.0]
          stringify(packed) := 129 251 64 89 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_capital_e_pos_exp.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 66 200 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_pos_exponent.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_pos_exponent.json: output to output adapters
          j1 := [100.0]
          stringify(packed) := 129 251 64 89 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_pos_exponent.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 66 200 0 0 

../test/src/unit-cbor.cpp:2076: ERROR: CHECK( vec == packed ) is NOT correct!
  values: CHECK( {?} == {?} )
  logged: filename := ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_underflow.json
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_underflow.json: output to output adapters
          j1 := [0.0]
          stringify(packed) := 129 251 0 0 0 0 0 0 0 0 
          ~/json/cmake-build-debug/json_test_data/nst_json_testsuite/test_parsing/y_number_real_underflow.json: output adapters: std::vector<uint8_t>
          stringify(vec) := 129 250 0 0 0 0

The numbers 0.0, -0.0, 200, and 100 can be represented exactly by double and float. We just need to adjust the test cases.

nlohmann · 2020-05-12T10:57:59Z

(see http://cbor.me for a helpful tool)

dota17 · 2020-05-12T12:34:15Z

(see http://cbor.me for a helpful tool)

Thank you for the detailed log info and the tool, that really helps.

rvjr · 2020-05-12T20:32:58Z

I didn't look into the testcase but just quickly checked what happens in the conversions with this tiny piece of code:

void testConversion() {
	std::vector<double> values = {0.0, -0.0, 0e+1, 0e1, 123.456e-789, 20e1, 1E+2, 1e+2, 123e-10000000};
	for (int i = 0; i < (int)values.size(); i++) {
		double v = values[i];
		float vf = (float)v;
		double vd = (double)vf;
		printf(format("values[%d]==%f is a %s\n", i, v, vd == vf ? "float" : "double"));
	}
}

and this is the output:

values[0]==0.000000 is a float
values[1]==-0.000000 is a float
values[2]==0.000000 is a float
values[3]==0.000000 is a float
values[4]==0.000000 is a float
values[5]==200.000000 is a float
values[6]==100.000000 is a float
values[7]==100.000000 is a float
values[8]==0.000000 is a float

So they are all stored as floats. I think everything is working fine; the point is just that 123.456e-789 and 123e-10000000 cannot be stored lossless in a double and have zero as closest representation in a 64 bit double value, so we can just as well store them as float zero (which is also correct in my opinion).

include/nlohmann/detail/output/binary_writer.hpp

dota17 · 2020-05-13T12:46:11Z

clean commit and rebase.

nlohmann

Looks good to me.

coveralls · 2020-05-13T18:14:43Z

Coverage increased (+0.0002%) to 99.941% when pulling ed9c205 on dota17:issue#1719 into bcf4f3c on nlohmann:develop.

nlohmann · 2020-05-13T18:50:44Z

There is a warning from UBSAN which breaks the build, see https://travis-ci.org/github/nlohmann/json/jobs/686552145#L915:

      Start  7: test-cbor
912
9137: Test command: /home/travis/build/nlohmann/json/build/test/test-cbor "--no-skip"
9147: Test timeout computed to be: 2700
9157: ../single_include/nlohmann/json.hpp:12131:62: runtime error: 1.79769e+308 is outside the range of representable values of type 'float'
9167: ==7080==WARNING: invalid path to external symbolizer!
9177: ==7080==WARNING: Failed to use and restart external symbolizer!
9187:     #0 0x7d0aac  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x7d0aac)
9197:     #1 0x7d3bf3  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x7d3bf3)
9207:     #2 0x7cccab  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x7cccab)
9217:     #3 0x7315a7  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x7315a7)
9227:     #4 0x5f1d67  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x5f1d67)
9237:     #5 0x5f844c  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x5f844c)
9247:     #6 0x7f34ce04ef44  (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
9257:     #7 0x4e5c3b  (/home/travis/build/nlohmann/json/build/test/test-cbor+0x4e5c3b)
9267: 
9277: SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../single_include/nlohmann/json.hpp:12131:62 in 
928 7/49 Test  #7: test-cbor ..........................***Failed   57.38 sec

nlohmann · 2020-05-13T18:51:06Z

Any ideas?

rvjr · 2020-05-13T19:32:55Z

Well this is exactly what this line is supposed to do:

const float value_float = static_cast<float>(value_double);

It should convert to float, as good as possible. And if there is no representation then we should detect this as not being representable as float, which is the intended use of the cast.

dota17 · 2020-05-14T04:52:43Z

I am confusing, in json_test_data, there is no such input data like 1.79769e+308.
It only exists in unit-testsuites.cpp, for double-number compliance tests, not a cbor conversion.

nlohmann · 2020-05-14T05:39:56Z

I think the problem occurs when we handle a NaN or infinity value. We should first handle these cases and only then convert the value to float.

That is, move the lines

const auto value_double = j.m_value.number_float;
const float value_float = static_cast<float>(value_double);

to the else branch.

nlohmann · 2020-05-14T10:59:31Z

That still does not fix it. Did you try to run with UBSAN locally? Would be interesting to see which test fails.

dota17 · 2020-05-14T11:56:54Z

That still does not fix it. Did you try to run with UBSAN locally? Would be interesting to see which test fails.

Just run it with UBSAN in local, the error still appeared:

 7/49 Test  #7: test-cbor ..........................***Failed   81.58 sec
/usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/stl_bvector.h:158:20: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/stl_bvector.h:158:20 in
/data/jsonformodernc/llc/json/single_include/nlohmann/json.hpp:12146:64: runtime error: 1.79769e+308 is outside the range of representable values of type 'float'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /data/jsonformodernc/llc/json/single_include/nlohmann/json.hpp:12146:64 in

nlohmann · 2020-05-14T12:00:21Z

You could run the test-cbor binary directly and check in the debugger in which line the error occurs.

dota17 · 2020-05-14T12:23:10Z

Well, it still at
static_cast<double>(static_cast<float>(j.m_value.number_float)) == j.m_value.number_float

nlohmann · 2020-05-14T12:23:50Z

And which input?

dota17 · 2020-05-14T12:26:51Z

And which input?

1e+300

nlohmann · 2020-05-14T12:35:32Z

Then I think we need to compare against std::numeric_limits<float>::max/std::numeric_limits<float>::min before trying to convert.

dota17 · 2020-05-14T13:00:36Z

Done, no more undefined-behavior in my local.

nlohmann · 2020-05-14T13:06:41Z

Could you also test against min() and add some unit tests?

nlohmann

PS: When you always rebase/squash your commits, reviewing becomes very hard as I never see a delta from the last commit, but need to start fresh.

include/nlohmann/detail/output/binary_writer.hpp

test/src/unit-cbor.cpp

nlohmann

Looks good to me.

nlohmann · 2020-05-16T10:57:05Z

Thanks!

nlohmann · 2020-05-16T10:57:41Z

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

Description

Use float representation in CBOR output if possible to save space. Use float and possibly half in json::to_cbor #1719 Fix issue#1719 #2044

dota17 requested a review from nlohmann as a code owner April 16, 2020 03:04

nlohmann added the aspect: binary formats BSON, CBOR, MessagePack, UBJSON label Apr 18, 2020

nlohmann linked an issue Apr 18, 2020 that may be closed by this pull request

Use float and possibly half in json::to_cbor #1719

Closed

nlohmann requested changes Apr 18, 2020

View reviewed changes

dota17 force-pushed the issue#1719 branch from 91c2080 to 0ae471c Compare May 6, 2020 06:46

nlohmann requested changes May 6, 2020

View reviewed changes

nlohmann reviewed May 7, 2020

View reviewed changes

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

dota17 force-pushed the issue#1719 branch from b87b962 to a638406 Compare May 8, 2020 09:38

dota17 mentioned this pull request May 12, 2020

Use float and possibly half in json::to_cbor #1719

Closed

nlohmann mentioned this pull request May 13, 2020

Update some cbor data nlohmann/json_test_data#1

Merged

nlohmann requested changes May 13, 2020

View reviewed changes

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

dota17 force-pushed the issue#1719 branch from 5ec7fe5 to 8fce1c9 Compare May 13, 2020 12:43

nlohmann approved these changes May 13, 2020

View reviewed changes

nlohmann self-assigned this May 13, 2020

nlohmann added this to the Release 3.8.0 milestone May 13, 2020

nlohmann added the release item: ⚡ improvement label May 13, 2020

dota17 added 3 commits May 14, 2020 15:10

Enhace to_cbor() to support +/-Infinity, NaN, and single-precision float

44fe284

Add some test cases about to_cbor()

47c6570

use json_test_data version 2.0.0

8f5b5c7

dota17 force-pushed the issue#1719 branch from 8fce1c9 to 8f5b5c7 Compare May 14, 2020 07:15

fix UBSAN

e175150

update

779a0ec

dota17 force-pushed the issue#1719 branch from 5722544 to 779a0ec Compare May 15, 2020 09:36

nlohmann requested changes May 15, 2020

View reviewed changes

include/nlohmann/detail/output/binary_writer.hpp Outdated Show resolved Hide resolved

test/src/unit-cbor.cpp Show resolved Hide resolved

dota17 added 2 commits May 15, 2020 21:47

compare against max float and min float before converting

5dd27f1

add somes test cases

ed9c205

nlohmann approved these changes May 15, 2020

View reviewed changes

nlohmann merged commit f40a9f8 into nlohmann:develop May 16, 2020

dota17 deleted the issue#1719 branch May 18, 2020 00:51

nlohmann mentioned this pull request Jun 19, 2020

Serialize floating-point numbers with 32 bit when possible (MessagePack) #2201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue#1719 #2044

Fix issue#1719 #2044

dota17 commented Apr 16, 2020 •

edited

Loading

rvjr commented Apr 16, 2020

nlohmann left a comment

nlohmann commented May 2, 2020

dota17 commented May 6, 2020

nlohmann left a comment

nlohmann left a comment

dota17 commented May 8, 2020

nlohmann commented May 12, 2020

nlohmann commented May 12, 2020

dota17 commented May 12, 2020 •

edited

Loading

rvjr commented May 12, 2020

dota17 commented May 13, 2020

nlohmann left a comment

coveralls commented May 13, 2020 •

edited

Loading

nlohmann commented May 13, 2020

nlohmann commented May 13, 2020

rvjr commented May 13, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

nlohmann left a comment

nlohmann left a comment

nlohmann commented May 16, 2020

nlohmann commented May 16, 2020

Fix issue#1719 #2044

Fix issue#1719 #2044

Conversation

dota17 commented Apr 16, 2020 • edited Loading

Pull request checklist

Describe

rvjr commented Apr 16, 2020

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented May 2, 2020

dota17 commented May 6, 2020

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann left a comment

Choose a reason for hiding this comment

dota17 commented May 8, 2020

nlohmann commented May 12, 2020

nlohmann commented May 12, 2020

dota17 commented May 12, 2020 • edited Loading

rvjr commented May 12, 2020

dota17 commented May 13, 2020

nlohmann left a comment

Choose a reason for hiding this comment

coveralls commented May 13, 2020 • edited Loading

nlohmann commented May 13, 2020

nlohmann commented May 13, 2020

rvjr commented May 13, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

dota17 commented May 14, 2020

nlohmann commented May 14, 2020

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented May 16, 2020

nlohmann commented May 16, 2020

🔖 Release item

Type

Description

dota17 commented Apr 16, 2020 •

edited

Loading

dota17 commented May 12, 2020 •

edited

Loading

coveralls commented May 13, 2020 •

edited

Loading