Extending support to full uint64_t/int64_t range - issue #178 (Integer conversion to unsigned)

[twelsby]

This pull request is to address issues raised in issue #178 related to the use of uint64_t and int64_t types and the inaccuracies involved in parsing, or creating objects with the full range of these values. Specifically it addresses the following problems:

1. Creating an object with a uint64_t value greater than (2⁶³) -1 will cause it to be represented as an incorrect negative value internally, and subsequently serialized as the negative value.
2. Numbers are parsed as a double initially and then converted to integers if possible, so parsing a number greater than (2⁵³) - 1 or less than -(2⁵³) + 1 will result in loss of precision. While this behavior is actually endorsed by RFC7159, due to the common use of uint64_t/int64_t types in modern software it would be preferable if the full range could be supported with full precision.

The pull request contains extensive changes to the code base however the majority of these are merely to introduce a new type, number_unsigned_t, which is required to support the upper range of uint64_t/int64_t values. The flow on effect of this is the addition of companion constructors, operators and miscellaneous code that is required to implement the new type. There are also very substantial changes to the unit testing (now just shy of 8000 assertions), again to accommodate the new type. This new code is in the same form as that for the existing types so there will be no surprises. A small number of additional unit tests check the performance of the parsing against the full range of the uint64_t/int64_t types.

The most controversial change is the change to the number parsing function get_number(). This now attempts to parse the number both as an integer using std::strloull()/std::strtoll() and also using std::strtold() and then stores using the best representation. This method was presented in the discussion for issue #178.

This method is far from ideal however the most likely alternatives seem to be either writing some custom (non-standard library based) number parsing function, or attempting to scan ahead in the string before parsing, or modifying the lexer to make the determination, or just accepting the reduced range. Neither of these alternatives seems ideal.

[nlohmann]

Hi @twelsby, thanks for your work! I'll have a look at the diff. In the meantime, could you check why the unit tests fail on Travis?

[nlohmann]

In fact, I used "iff" as (common?) abbreviation for "if and only iff" here.

[twelsby]

Sorry I didn't know that one and thought it was a typo.

[nlohmann]

I looked at the diffs and adde some comments. Wow - I am impressed how much effort you put to extend all the unit tests!

[Furcube]

Awesome amount of changes.

A little addition: you should set errno = 0 before calling strto functions.
From linux man page for a strtoull:

NOTES
       Since strtoul() can legitimately return 0 or LONG_MAX (LLONG_MAX for strtoull()) on both success and failure, the calling program should set errno to 0 before the call, and then determine if an  error
       occurred by checking whether errno has a non-zero value after the call.

I assume it does not change errno on success.

[twelsby]

Good pickup @Furcube I completely forgot about that. I'll fix it.

[Furcube]

result.m_value.number_integer = std::strtoll

This will be very bad if NumberIntegerType is not a int64_t, but int32_t for example
Need to change conversion function to appropriate or do range check additionaly to ERANGE.

[twelsby]

@Furcube, another good point. I think the best way would be to store it in a temporary int64_t /uint64_t, check the range of NumberIntegerType and cast to either NumberIntegerType or NumberFloatType as appropriate. One extra if/else, so not a huge problem. This is where it would be nice to have a templated conversion function.

[twelsby]

The latest commit has the following changes:

1. Fixed the errno issue identified by @Furcube
2. Fixed the issue with small integers identified by @Furcube
3. Cleaned get_number() up a bit to increase code reuse using a helper function
4. Implemented the fix referred to in issue MSVC 2015 build fails when attempting to compare object_t #167 - AppVeyor now passes
5. Removed the use of approx as discussed in issue Floating point equality #185. Understandably this will cause angst for some but as I stated in the issue it isn't doing what it is designed for and what it is designed for shouldn't be done. I look forward to comments on the issue and have no issues continuing with the current way of doing things if any concerns cannot be placated.

[twelsby]

Latest build adds a fix for #171 by adding two extra template overloads for operator[] for T* arguments. Seems to work well with no apparent side effects.

[nlohmann]

I think this part with the #ifdefs and #pragmas is really ugly... I am not 100% convinced yet to have something like this in the library...

[twelsby]

This is necessary to suppress floating point warnings under gcc/clang. The only real alternatives are to:

1. Accept the warnings - they are only warnings after all (ugly though)
2. Change the build settings in the makefile so that -Wfloat-equal is omitted at the command line - this works for us but if the user decides to compile with the warning, and doesn't understand floating point, then they may come to the conclusion that this is a bug in our code
3. Add the #pragma without the conditional block - this creates its own warning about unknown #pragma under VS2015
4. Reinstate approx()
5. Come up with some other means of avoiding the warning if such a thing exists

I will revert this for now and add it to its own pull request as it is really a separate issue.

[nlohmann]

Hi @twelsby, could you please check #186 with your get_number function?

[twelsby]

@nlohmann, can you please close this pull request. I accidentally created it from my master which is causing all sorts of problems as I would like to point master elsewhere but can't while this is open. I will create another pull request with just the changes relevant to this subject and add a reference to this one. Unfortunately github does not allow changing the pull request source branch (although its an open feature request).

[nlohmann]

Closed as requested by @twelsby.