Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Utilities for decimal <--> floating conversion #15359
Utilities for decimal <--> floating conversion #15359
Changes from 18 commits
31aec0e
d3332f5
84d9be8
0e65c39
45ed607
359a9f7
3529d8d
45ee1bd
107a86f
309566c
f4f5819
41b205f
ec398c3
eeed7bc
9c09519
59d6edd
b2cc813
d3348cf
77c84c3
150a115
dcc4819
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? If the sign bit is already
1
, it's not possible to change it with an OR operator, regardless ofis_negative
. Are we assuming the sign bit is always0
when entering this function? If so, that is not documented.Are there test cases for this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sign is always zero on input, I'll add that comment. It's not part of this PR, but when I switch from the old decimal/floating conversion code to this, the fixed_point tests test all of this functionality, and all of those tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might I suggest a reminder in the comment added that 0 means positive value, so the or will do what we want. Simply saying it must be positive implies the reader knows this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the design of this function.
is_negative
is true, this should be a no-op -- we shouldn't even cast back and forth.return is_negative ? -floating : floating;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR contains utilities for the upcoming primary PR for the decimal <--> floating conversion. I broke the code into several PRs as it is quite large. So nothing is calling this yet.
This specific function is used at the end of decimal --> floating. In the main algorithm we do a lot of bit-shifting so the sign is initially cleared, and here we are setting it at the end. Doing it this way (cast + or) requires no branching, so you don't pay a performance penalty for the branch. A couple months ago I did a bunch of benchmarking trying different methods and this method was the fastest.