Fix the DurationFormat fractionl output [v8/v8 : main]
Jakob Linke (Gerrit)
Jakob Linke added 9 comments![]( "Open in Gerrit")
cc Manish and Leszek, please do chime in if you have comments. I did not review against the spec in this round, Manish would you be the right person to do so?
bool OutputSimpleNumber(const char* type, int64_t value, size_t powerOfTen,Please comment what the difference is to OutputLongShortOrNarrow, and which part of the spec this implements.
// k. If value is not 0 or display is not "auto" or displayRequired is "true",This last part is assumed false?
// return Output(type, value, fmt, addToLast, display_negative_sign,Leftover
if (digits.length() < powerOfTen) {Let's add a `DCHECK_LE(powerOfTen, 9)` to be safe.
return display_negative_sign;Why return anything if the result is unused below?
record.time_duration.seconds * 1000000000LL;What protects against overflow here and below?
// Test the fractional operation is calculate in Math Value not double.Please explain what "Math Value" means. A link to https://tc39.es/ecma262/#sec-mathematical-operations would help.
{hours: 0, minutes: 0, seconds: 2, milliseconds: 712}));Could you add more tests covering edge cases, e.g. for large values.
Related details
- Frank Tang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Tang (Gerrit)
Frank Tang added 3 comments![]( "Open in Gerrit")
Please do NOT look at it yet. We found some more problem. Fixing it now.
Let's add a `DCHECK_LE(powerOfTen, 9)` to be safe.
Done
Why return anything if the result is unused below?
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar added 1 comment![]( "Open in Gerrit")
cc Manish and Leszek, please do chime in if you have comments. I did not review against the spec in this round, Manish would you be the right person to do so?
I'm not super familiar with this spec but I can verify against it if you'd like. Unfortunately I have a hard time figuring out what parts of the spec this implements: the spec text comments in the new function do not seem to be anywhere in ecma402 or proposal-intl-duration-format (which one should we be using here?), and I'm also having a hard time figuring out what spec sections the existing edited code is for. Perhaps https://tc39.es/ecma402/#sec-formatnumericseconds ?
Related details
- Frank Tang
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Tang (Gerrit)
Frank Tang added 7 comments![]( "Open in Gerrit")
ok, ready for another round of review. PTAL
bool OutputSimpleNumber(const char* type, int64_t value, size_t powerOfTen,Please comment what the difference is to OutputLongShortOrNarrow, and which part of the spec this implements.
Done
// k. If value is not 0 or display is not "auto" or displayRequired is "true",This last part is assumed false?
Done
// return Output(type, value, fmt, addToLast, display_negative_sign,Frank TangLeftover
Done
What protects against overflow here and below?
!temporal::IsValidDuration(isolate, record)
should throw before reaching this point
// Test the fractional operation is calculate in Math Value not double.Please explain what "Math Value" means. A link to https://tc39.es/ecma262/#sec-mathematical-operations would help.
Done
Could you add more tests covering edge cases, e.g. for large values.
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Tang (Gerrit)
Frank Tang added 1 comment![]( "Open in Gerrit")
// Use faster ICU API formatInt if the vaue fit the precision int64_t,Please fix this WARNING reported by Spellchecker: "vaue" is a possible misspelling of "value".
To bypass Spellchecker, add a foot...
"vaue" is a possible misspelling of "value".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Related details
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Code-Review | +1 |
Manish Goregaokarcc Manish and Leszek, please do chime in if you have comments. I did not review against the spec in this round, Manish would you be the right person to do so?
I'm not super familiar with this spec but I can verify against it if you'd like. Unfortunately I have a hard time figuring out what parts of the spec this implements: the spec text comments in the new function do not seem to be anywhere in ecma402 or proposal-intl-duration-format (which one should we be using here?), and I'm also having a hard time figuring out what spec sections the existing edited code is for. Perhaps https://tc39.es/ecma402/#sec-formatnumericseconds ?
Yes that is the spec section I found as well. A second look would be appreciated since my familiarity here is zero :)
Rubberstamp +1 from my side.
// k. If value is not 0 or display is not "auto" or displayRequired is "true",Frank TangThis last part is assumed false?
Done
- Frank Tang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar added 1 comment![]( "Open in Gerrit")
Manish Goregaokarcc Manish and Leszek, please do chime in if you have comments. I did not review against the spec in this round, Manish would you be the right person to do so?
Jakob LinkeI'm not super familiar with this spec but I can verify against it if you'd like. Unfortunately I have a hard time figuring out what parts of the spec this implements: the spec text comments in the new function do not seem to be anywhere in ecma402 or proposal-intl-duration-format (which one should we be using here?), and I'm also having a hard time figuring out what spec sections the existing edited code is for. Perhaps https://tc39.es/ecma402/#sec-formatnumericseconds ?
Yes that is the spec section I found as well. A second look would be appreciated since my familiarity here is zero :)
Yeah, the code doesn't really match well so I'm not sure how to verify it against the spec. Comments pointing out spec steps would be really nice to have.
Related details
- Frank Tang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |