src: move JSDate "ToString" logic to JSDate class #257
Conversation
| return pre + s + ">"; | ||
| } | ||
|
|
||
| double d = static_cast<double>(val.raw()); |
There was a problem hiding this comment.
@evanlucas do you remember why this last part, casting to double, was added? I couldn't find any test case where I could reach it. It was originally introduced in https://github.com/nodejs/llnode/pull/3/files
There was a problem hiding this comment.
not off the top of my head, sorry!
mmarchini
added
the
author ready
src/llv8-inl.h
Outdated
|
|
||
| ACCESSOR(JSDate, GetValue, js_date()->kValueOffset, Value) | ||
|
|
||
| bool JSDate::IsSmi(Error& err) { |
There was a problem hiding this comment.
These two helpers seem unnecessary if the call site need to grab the values out either ways?
There was a problem hiding this comment.
I like the idea of having IsTYPE methods instead of duplicating the tests everywhere. If V8 changes how we check for a type we only need to make changes to one place. But I'd rather have all IsTYPE methods in the Value class, so it resembles the V8 API (https://github.com/nodejs/llnode/pull/247/files#diff-6863f2f9f81092029c00ed6e07b78700R60).
There was a problem hiding this comment.
But looking again those two don't add much value, since they're just calling .Check...
We might want to settle on style here (TYPE.Check() vs. Value.IsTYPE()), but we don't have to decide in this PR.
mmarchini
left a comment
mmarchini
left a comment
There was a problem hiding this comment.
LGTM, but ToString should be moved to the Printer class.
|
Ignore my previous comment, I've mistaken ToString with the old |
Move logic that was representing a JSDate object to a string from the Printer::Stringfy method to the JSDate class, implementing "ToString" method.
mmarchini
force-pushed
the
move_js_date_from_printer
branch
from
2521496 to
3912ebe
Compare
Codecov Report
@@ Coverage Diff @@
## master #257 +/- ##
==========================================
+ Coverage 79.96% 80.42% +0.46%
==========================================
Files 34 34
Lines 4292 4292
==========================================
+ Hits 3432 3452 +20
+ Misses 860 840 -20
Continue to review full report at Codecov.
|
|
Daniel is on vacation, so I added a commit to address the nit. |
Move logic that was representing a JSDate object to a string from the Printer::Stringfy method to the JSDate class, implementing "ToString" method. PR-URL: #257 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
|
Landed in ecfbdca |
5 participants
Move logic that was representing a JSDate object
to a string from the Printer::Stringfy method to
the JSDate class, implementing "ToString" method.