Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issues with reading/writing JSON floats #56

Merged
merged 3 commits into from
Sep 24, 2019
Merged

Conversation

snej
Copy link
Contributor

@snej snej commented Sep 23, 2019

This PR does the same as #51, basically, but brings in a lot less 3rd party code.

@snej snej self-assigned this Sep 23, 2019
@Dushistov
Copy link
Contributor

One thing that is bad, that in compare with google/double-conversation there are no included unit tests that you can run on every platform and every compiler, it should be fine for Mac OS/iOS,
but what about other? Almost nobody uses swift on Linux/Windows/Android even if there is port to that platform.

@snej snej force-pushed the fix/ya-float-fix branch 2 times, most recently from 37bea44 to 03b3653 Compare September 24, 2019 00:24
@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

there are no included unit tests that you can run on every platform and every compiler,

True. But I still chose to use this library over the larger Google one.

it should be fine for Mac OS/iOS,
but what about other? Almost nobody uses swift on Linux/Windows/Android even if there is port to that platform.

Not really. There's a nontrivial Swift community on Linux; IBM is a big backer of server-side Swift. And Swift itself is well-tested.

@snej snej force-pushed the fix/ya-float-fix branch 3 times, most recently from 10e5574 to 54661d8 Compare September 24, 2019 00:55
* Reading/writing floating point was accidentally locale-dependent,
  which caused errors in locales that don't use '.' as the decimal
  point. (Fixes #54)
* Improved round-trip accuracy of Fleece->JSON->Fleece float/double
  conversions, by writing the optimal number of decimal places.
  To do this we brought in a small subcomponent of Swift. (Fixes #55)
@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

It's alllllmost building on Windows. MSVC is just complaining about the literal 0x1.0p24F, which I have to admin confuses me too. It looks like this is a hex representation of a float‽

@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

... yup, it's a hexadecimal floating-point literal, and the p is the binary equivalent of e in a regular float. And my suspicion was correct, this is a C99 feature which is why MSVC doesn't like it.

The good news is that this syntax was added to C++17 so we can work around the problem by bumping the compiler dialect to C++17 ... see, @borrrden, I found a good reason to switch! 🤣

@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

Oh wonderful — Clang 5 claims to support C++17 (__cplusplus >= 201700L is true) but doesn't have string_view.

@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

Apparently MSVC doesn't support hex float literals at all. There's no mention of them in their docs.

@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

According to someone on SO, "Visual C++ will have [hex float literals] available in Visual Studio 2017 15.6 release."
Whereas the logs say our Appveyor build uses Visual Studio 14.0. :(

MSVC doesn't support them, at least not the version we build with
@snej
Copy link
Contributor Author

snej commented Sep 24, 2019

I backed out the C++17 stuff and just changed the hex FP literals to regular ones.
Also added a subset of the Swift unit tests for the formatter.

@borrrden
Copy link
Member

borrrden commented Sep 24, 2019

Yes, here is the official blog that says the feature is available in 15.5 (aka VS 2017). We can change AppVeyor to use that, but we also need to change our entire build infrastructure to move from VS2015 to VS2017 (something that eventually we will need to do)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants