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

Issue 13013 - Converted real to double in std.json.JSONValue #2291

Merged
merged 1 commit into from
Jul 3, 2014
Merged

Issue 13013 - Converted real to double in std.json.JSONValue #2291

merged 1 commit into from
Jul 3, 2014

Conversation

markisaa
Copy link
Contributor

@markisaa markisaa commented Jul 1, 2014

https://issues.dlang.org/show_bug.cgi?id=13013

  1. When compiling with -m64 on Windows (when using the MSVC linker), the real type is unsupported. std.json unittests fail with the version of std.json in master because of this.
  2. JSON only uses 64-bit floating point

=> Changed real to double, as it is more functional and no less correct.

One concern: ABI compatibility.

Tested this on both Windows and OSX.

@@ -58,7 +58,7 @@ struct JSONValue
string str;
long integer;
ulong uinteger;
real floating;
double floating;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignment

@yebblies
Copy link
Member

yebblies commented Jul 2, 2014

One concern: ABI compatibility.

ABI compatibility is not a concern for phobos.

@dnadlinger
Copy link
Member

ABI compatibility can't be a concern at the current point at all. We don't even have a well-defined ABI to start with.

@markisaa
Copy link
Contributor Author

markisaa commented Jul 2, 2014

Addressed the inline comment. The ABI mention was mostly to continue that train of thought from the Bug Report page. As best as I can tell, this PR is now ready.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 3, 2014

Looks good to me.

@dnadlinger
Copy link
Member

Auto-merge toggled on

dnadlinger added a commit that referenced this pull request Jul 3, 2014
Issue 13013 - Converted real to double in std.json.JSONValue
@dnadlinger dnadlinger merged commit f518343 into dlang:master Jul 3, 2014
@dnadlinger
Copy link
Member

Okay, thanks. Should be good to go – we'll see if the type changes lead to a compatibility problem in an unexpected way during beta, I guess.

@markisaa markisaa deleted the realToDouble branch July 3, 2014 17:38
9rnsr pushed a commit to 9rnsr/phobos that referenced this pull request Jul 7, 2014
Issue 13013 - Converted real to double in std.json.JSONValue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants