dropbox / json11

A tiny JSON library for C++11.
MIT License
2.54k stars 613 forks source link

Fails compiler checks for comparing floating point #103

Closed crashmatt closed 7 years ago

crashmatt commented 7 years ago

If you have compile options set to throw a compile error on floating point comparison then there is trouble here:

I don't know a suitable template specialization that works here. That does mean there is not one. https://github.com/dropbox/json11/blob/master/json11.cpp#L163

This needs an "epsilon" comparison. https://github.com/dropbox/json11/blob/master/json11.cpp#L176

and on the int templated https://github.com/dropbox/json11/blob/master/json11.cpp#L185 which is strange but easy to fix with a type cast.

Will submit a PR if I find a good fix for everything.

j4cbo commented 7 years ago

Comparisons on floating point numbers can be dangerous, but it's better for json11 to implement comparison as defined by the compiler (IEEE 754, etc) than to introduce potentially-surprising behavior like an epsilon. I think the right fix here is to disable the warning when compiling json11 or, if your compiler supports it, add some comments to suppress the warning.

crashmatt commented 7 years ago

I am stuck with a project that enforces no float comparisons. Tried finding a way to suppress the compile error and failed. Is likely I didn't find the right way. If I were to suggest anything I would make it a defined option. I am now using a hacked up json11 version just to get me going.

Thanks for your comment. Closing for now.

/Matt

j4cbo commented 7 years ago

One option would be switching to std::equal_to() - is your compiler happy with that?

crashmatt commented 7 years ago

std::equal_to is directly equivalent to ==

std::equal_to() might be overridden for double. It would be nice if the user could override equal_to outside of json11.cpp but I have a hunch that won't work.

Something to work on....

On 24 April 2017 at 01:20, j4cbo notifications@github.com wrote:

One option would be switching to std::equal_to() - is your compiler happy with that?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/dropbox/json11/issues/103#issuecomment-296496335, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVcHWlH3nY67GQlJjDopnjsWxgReJaCks5ry9ysgaJpZM4M-4Lo .

artwyman commented 7 years ago

What compiler are you using? Most of them have options to suppress warnings, and I don't think it would be unreasonable for us to put those warnings in the code here, so long as they were safely wrapped in #ifdefs.

j4cbo commented 7 years ago

For clang, #pragma clang diagnostic ignored "-Wfloat-equal" should do it.

crashmatt commented 7 years ago

Thanks for the hint. My failed search attempts didn't find that.

I am using gcc but some users are compiling the same code with clang. There is the gcc equivalent: https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

Which is pretty much the same thing. There is also:

pragma GCC diagnostic push

pragma GCC diagnostic pop

which hopefully works for clang also.

Will wrap it up in a nice #ifdef and submit a PR.