Uh oh!
There was an error while loading. Please reload this page.
Feature GH-12143: Extend the maximum precision round can handle by one digit - #12222
Closed
SakiTakamachi wants to merge 1 commit into
Closed
Feature GH-12143: Extend the maximum precision round can handle by one digit#12222SakiTakamachi wants to merge 1 commit into
SakiTakamachi wants to merge 1 commit into
Conversation
TimWolla
commented
Sep 19, 2023
Member
@SakiTakamachi Can you rebase this, now that #12220 is merged? |
SakiTakamachiforce-pushed
the
feature/expand-rounding-target
branch
from
September 20, 2023 00:06
082e637 to
e330e0bCompareSakiTakamachi
commented
Sep 20, 2023
MemberAuthor
@TimWolla |
SakiTakamachi
commented
Sep 20, 2023
MemberAuthor
The digit adjustment is not going well, I am currently fixing it. |
SakiTakamachi
commented
Sep 20, 2023
MemberAuthor
This is a malfunction caused by expanding the accepted digits, so there is no problem with the digit adjustment fix PR itself. |
SakiTakamachiforce-pushed
the
feature/expand-rounding-target
branch
4 times, most recently
from
February 10, 2024 14:17
31304be to
6e9ade7CompareSakiTakamachi
marked this pull request as ready for review
February 10, 2024 14:17
SakiTakamachiforce-pushed
the
feature/expand-rounding-target
branch
from
February 10, 2024 14:44
6e9ade7 to
fb41f0fCompareGirgias
reviewed
Feb 13, 2024
Girgias
left a comment
Member
There was a problem hiding this comment.
One thing that would make it easier to see what changes in these sorts of PRs is to use the rules LLVM has for PRs.
First commit the test with the current behaviour, then do a follow-up commit with the C changes and how it impacts the test.
SakiTakamachi
commented
Feb 13, 2024
MemberAuthor
Thanks, I'll try that |
SakiTakamachi
commented
Feb 15, 2024
MemberAuthor
SakiTakamachiforce-pushed
the
feature/expand-rounding-target
branch
from
February 21, 2024 23:48
fb41f0f to
efdc240CompareGirgias
approved these changes
Feb 22, 2024
Girgias
left a comment
Member
There was a problem hiding this comment.
LGTM, could you add an UPGRADING note when merging?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please check here for more details.
#12143 (comment)
In the existing implementation, for example, the following code does not perform rounding:
In this example, the maximum value that can be specified for
round()argument$precisionis15.This pull request changes
round()to accept one more fine digit. (16for the above example)The test will probably not succeed unless it incorporates the changes in #12220.
Also, this PR also includes changes in #12162.
Therefore, you may not be able to accurately check this PR until after these two changes have been merged.