-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[Temporal] Add PlainYearMonth from method
#54800
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
[Temporal] Add PlainYearMonth from method
#54800
Conversation
|
EWS run on previous version of this PR (hash aff2a2f) Details |
aff2a2f to
f5d2caf
Compare
|
EWS run on previous version of this PR (hash f5d2caf) Details |
|
EWS run on previous version of this PR (hash 1a4c1ed) Details |
| auto dateTime = ISO8601::parseCalendarDateTime(string, TemporalDateFormat::YearMonth); | ||
| if (dateTime) [[likely]] { | ||
| auto [plainDate, plainTimeOptional, timeZoneOptional, calendarOptional] = WTFMove(dateTime.value()); | ||
| if (calendarOptional && StringView(calendarOptional.value()) != String::fromLatin1("iso8601")) [[unlikely]] { |
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.
Please do not use String::fromLatin1 for ASCII literals. We need to remove this from existing code and stop adding it in new code!
| if (calendarOptional && StringView(calendarOptional.value()) != String::fromLatin1("iso8601")) [[unlikely]] { | |
| if (calendarOptional && calendarOptional.value() != "iso8601"_s) [[unlikely]] { |
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.
Happy to take out the fromLatin1, but it won't compile without the StringView (calendarOptional.value() is a Vector<Latin1Character, maxCalendarLength>).
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.
Best idiom for comparing a Vector of Latin1Character with a string constant is to call the equal function that compares two spans, something like:
| if (calendarOptional && StringView(calendarOptional.value()) != String::fromLatin1("iso8601")) [[unlikely]] { | |
| if (calendarOptional && !equal(calendarOptional->span(), "iso8601"_span8)) [[unlikely]] { |
Using StringView brings branches for handling of 16-bit characters into play, unnecessarily.
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.
Done.
|
|
||
| DECLARE_INFO; | ||
|
|
||
| static TemporalPlainYearMonth* from(JSGlobalObject*, JSValue, std::optional<JSValue>); |
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 don’t see any code passing nullopt for this third argument. Are you sure that code path is exercised?
JSValue has a distinct zero value that is distinct from JavaScript null and undefined, so it can be used for a JSValue that might be omitted, passing zero value as appropriate. And often in other JavaScript contexts we can use null and undefined instead. Do we really need to use std::optional here? I can’t find where it is used and for what.
Typically when called from JavaScript, omitted arguments are represented with an JSValue of undefined.
Also, the argument meaning isn’t clear here without an argument name, so we should include that name here.
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 think the reason it was optional is that it's used that way in a future commit (I originally had a big monolithic PR for these changes and what I'm submitting now is the result of splitting it up), but using the zero value for JSValue seems fine.
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 suspect we might want to use undefined rather than the zero value; I’d probably have to see more of the future code to know if that’s a good choice. But I also would have probably suggested making it non-optional and then add the ability to leave it out in that future commit.
|
EWS run on previous version of this PR (hash 0950eb6) Details |
|
EWS run on previous version of this PR (hash b83a1d9) Details |
|
EWS run on previous version of this PR (hash 4c22618) Details |
|
Could you squash the commits now? |
4c22618 to
25e768d
Compare
|
EWS run on previous version of this PR (hash 25e768d) Details |
|
@darinadler Squashed. |
| } | ||
|
|
||
| JSObject* options = nullptr; | ||
| if (optionsValue) { |
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.
optionsValue is now just JSValue, so if (optionsValue) means checking if optionsValue is not zero value.
But I think what we should do here is checking if optionsValue is not undefined (or null) ?
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.
If my comment makes sense, we should add more tests to check this behavior
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 think you're right, checking for undefined makes sense, because that's what would be passed in from temporalPlainYearMonthConstructorFuncFrom if no options argument was provided. I changed it. I also added stress tests for null and explicit-undefined options objects.
|
EWS run on previous version of this PR (hash dd954e8) Details |
dd954e8 to
f924752
Compare
|
EWS run on previous version of this PR (hash f924752) Details |
|
EWS run on previous version of this PR (hash b46358f) Details |
|
EWS run on previous version of this PR (hash bebded5) Details |
|
EWS run on previous version of this PR (hash 11bbfd5) Details |
11bbfd5 to
1e13a17
Compare
|
EWS run on current version of this PR (hash 1e13a17) Details |
sosukesuzuki
left a comment
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.
r=me
https://bugs.webkit.org/show_bug.cgi?id=303503 Reviewed by Darin Adler and Sosuke Suzuki. Add this method. Co-authored-by: Darin Adler <33187212+darinadler@users.noreply.github.com> * JSTests/stress/temporal-plainyearmonth.js: (shouldThrow): * JSTests/test262/config.yaml: * Source/JavaScriptCore/runtime/TemporalCalendar.cpp: (JSC::TemporalCalendar::isoDateFromFields): * Source/JavaScriptCore/runtime/TemporalPlainYearMonth.cpp: (JSC::TemporalPlainYearMonth::from): * Source/JavaScriptCore/runtime/TemporalPlainYearMonth.h: * Source/JavaScriptCore/runtime/TemporalPlainYearMonthConstructor.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION): Canonical link: https://commits.webkit.org/304135@main
1e13a17 to
7af7345
Compare
|
Committed 304135@main (7af7345): https://commits.webkit.org/304135@main Reviewed commits have been landed. Closing PR #54800 and removing active labels. |
7af7345
1e13a17
🧪 api-ios🧪 gtk-wk2🛠 playstation