Fix MaxCoefficient static variable value in Decimal.cpp #6160
Conversation
Fix MaxCoefficient static variable value in Decimal.cpp https://bugs.webkit.org/show_bug.cgi?id=247514 Reviewed by NOBODY (OOPS!). Merge - https://src.chromium.org/viewvc/blink?view=revision&revision=174294 Fix MaxCoefficient static variable value in Decimal.cpp to match the Precision and the corresponding comment. Webkit uses 18 as precision and the comment says MaxCoefficient should be 18 '9's in hexadecimal. However, 0x16345785D89FFFF is 99999999999999999 (17 '9's), not 18 '9's. The right hexadecimal value should be 0XDE0B6B3A763FFFF. * Source/WebCore/platform/Decimal.cpp: Update "MaxCoefficient" static variable
|
EWS run on current version of this PR (hash 812e7e8) |
Ahmad-S792
added
New Bugs
| @@ -48,7 +48,7 @@ static int const ExponentMax = 1023; | |||
| static int const ExponentMin = -1023; | |||
| static int const Precision = 18; | |||
|
|
|||
| static const uint64_t MaxCoefficient = UINT64_C(0x16345785D89FFFF); // 999999999999999999 == 18 9's | |||
| static const uint64_t MaxCoefficient = UINT64_C(0xDE0B6B3A763FFFF); // 999999999999999999 == 18 9's | |||
There was a problem hiding this comment.
I wonder if this mistake has any effect. Would be nice to test it.
There was a problem hiding this comment.
Unfortunately, I don't have any idea on how to write test for this change top of my head, any idea or suggestions?
There was a problem hiding this comment.
@darinadler - I tried to look into on how to write some test for it but I think it would be difficult at my novice level. Should I close this PR and leave comment on bugzilla for someone else to land this with testcase? Thanks!
There was a problem hiding this comment.
You can land it without a test case.
In the end if we have no idea what the effect is, it’s not so great; we could be making things worse somehow even.
Would be good to seek out someone who has an idea what effect an incorrect maximum would have. But that need not delay merging this corrected constant.
There was a problem hiding this comment.
If I am not mistaken @cdumez did this change in Blink based on the commit, I think he might have some insight. If he can share any insight, it would be great else I would land it. I agree with you that without doing proper testing, we are just trying to shoot in the blind.
There was a problem hiding this comment.
@Ahmad-S792 My change in Blink had an associated API test changes. Your cherry-pick doesn't so I think that's the issue.
There was a problem hiding this comment.
Looks like Blink has API test coverage for Decimal while WebKit doesn't. Seems like we should introduce some Decimal API tests.
There was a problem hiding this comment.
What happened to std::decimal? Why can’t we use that?
There was a problem hiding this comment.
What happened to
std::decimal? Why can’t we use that?
Is this a thing? I found a proposal for std::decimal but it's not clear to me after some googling that it is part of the standard.
|
Managed to find failing testcase due to this and another change and fixing both in one go via PR 7908. |
812e7e8