-
Notifications
You must be signed in to change notification settings - Fork 99
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not off the top of my head, sorry!
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.
LGTM with a nit
src/llv8-inl.h
Outdated
@@ -279,6 +279,18 @@ ACCESSOR(JSRegExp, GetSource, js_regexp()->kSourceOffset, String) | |||
|
|||
ACCESSOR(JSDate, GetValue, js_date()->kValueOffset, Value) | |||
|
|||
bool JSDate::IsSmi(Error& err) { |
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.
These two helpers seem unnecessary if the call site need to grab the values out either ways?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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 |
Move logic that was representing a JSDate object
to a string from the Printer::Stringfy method to
the JSDate class, implementing "ToString" method.