Check we don't overflow when casting down integers during parsing

[ArnaudBienner]

Hi,

I've faced the following issue when using this library: I stored some numbers in small integers (because I expect to use values in a small interval in practice) but the static_cast made during parsing lead to invalid values because of the overflow.

I believe it would be nice to have this runtime check to notice possible overflows at runtime.

[gregmarr]

This if will never be true, unless you get some weird results from val and min/max having different types. You want || not &&. You probably also want min and max to have the same type as val.

You would also only need to do this test if ArithmeticTypeTarget and ArithmeticTypeSource are different types, and then only if Target is smaller than Source, or if they have different signed-ness.

It might also be better to do this as a pre-test that just throws on out of range, and leave the current lines alone for the actual assignments.

[nlohmann]

I am a bit concerned that adding overflow exceptions (though correct) are changing the behavior of the library in a breaking (and maybe also surprising) way.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @ArnaudBienner
Please read and follow the Contribution Guidelines.

[coveralls]

coverage: 100.0%. remained the same
when pulling da5800c on ArnaudBienner:develop
into 8c391e0 on nlohmann:develop.

[ArnaudBienner]

Thank you all for your feedback :)

Turns out the check was (by chance) working in the sample case I wrote for unit testing :( I should have been more careful and more exhaustive in my testing, sorry about that. I've added a few more unit tests.

In addition to fixing the check, I also change the code to:

• Not perform the check if the source type is smaller than the destination type: as discussed, this is not needed in this case. I didn't managed to use enable_if to have two different version of that function. In my PR I'm using if constexpr to achieve a similar effect, but this is a C++17 feature, even if I know this project is C++11: this was just to make a point. I can either drop it completely, or if you have a way to define macros to support multiple C++/compilers versions and features, we can do this as well.
• Always perform the check in case the source number is floating points: in this case the sizeof check is not relevant
• Ignore the check when dealing with infinity numbers: I realized when running unit tests this was already handled specifically in the code elsewhere