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

fix unnecessary rounding to float64 precision when JSON-marshaling durations #453

Merged
merged 1 commit into from
Nov 13, 2017
Merged

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented Nov 6, 2017

No description provided.

@dsnet dsnet requested a review from cybrcodr November 6, 2017 22:01
@dfawley
Copy link
Contributor Author

dfawley commented Nov 13, 2017

Ping - this should be pretty trivial.

@dsnet
Copy link
Member

dsnet commented Nov 13, 2017

It seems Herbie is OOO.

@dsnet dsnet merged commit 1e59b77 into golang:master Nov 13, 2017
Copy link

@awalterschulze awalterschulze left a comment

Choose a reason for hiding this comment

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

Could we add a test for a negative duration?
I think this might result in -2277.-100626394s for example.

@puellanivis
Copy link
Collaborator

“For durations of one second or more, a non-zero value for the nanos field must be of the same sign as the seconds field. Must be from -999,999,999 to +999,999,999 inclusive.” https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Duration

It looks like you’re right, @awalterschulze . And, the negative duration test is a good idea in all cases. Additionally, a negative less-than-one-second test is also going to be important as this would currently write as “0.-100626394s” for example.

It looks like we will need to extract the sign and absolute value of both of the integers… some quick and dirty code (not intended as complete solution, I just think in code)…

sign := ""
if s < 0 {
  s = -s
  sign = "-"
}
if ns < 0 {
  ns = -ns
  sign = "-"
}
x := fmt.Sprintf("%s%d.%09ds", sign, s, ns)

Even though it’s more work, in the end, this should still work better (no power-of-ten rounding issues) and be faster than using floating points… (?)

@awalterschulze
Copy link

I was just trying to merge this change into gogoprotobuf and ran my tests, which found the bug. I am really glad to hear its not my tests, but an actual bug. Looking forward to merge a fix when its ready.

@dfawley
Copy link
Contributor Author

dfawley commented Jan 22, 2018

This is my fault -- I'll send a PR to fix this in a few minutes.

@dfawley
Copy link
Contributor Author

dfawley commented Jan 22, 2018

#491

dsnet added a commit that referenced this pull request Jan 22, 2018
…aling durations (#453)"

The change does not handle negative values correctly.

This reverts commit 1e59b77.
dsnet added a commit that referenced this pull request Jan 22, 2018
…aling durations (#453)" (#493)

The change does not handle negative values correctly.

This reverts commit 1e59b77.
chyroc pushed a commit to chyroc/protobuf that referenced this pull request May 1, 2018
chyroc pushed a commit to chyroc/protobuf that referenced this pull request May 1, 2018
…aling durations (golang#453)" (golang#493)

The change does not handle negative values correctly.

This reverts commit 1e59b77.
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants