WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
200072
[ESNext] Implement nullish coalescing
https://bugs.webkit.org/show_bug.cgi?id=200072
Summary
[ESNext] Implement nullish coalescing
Ross Kirsling
Reported
2019-07-23 22:10:46 PDT
[ESNext] Implement nullish coalescing
Attachments
Patch
(16.76 KB, patch)
2019-07-23 22:29 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch
(16.30 KB, patch)
2019-07-23 23:25 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch
(18.86 KB, patch)
2019-07-24 09:47 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch
(18.86 KB, patch)
2019-07-24 12:50 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch
(19.02 KB, patch)
2019-07-24 15:08 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch for landing
(16.32 KB, patch)
2019-07-24 20:52 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch for landing
(16.33 KB, patch)
2019-07-24 21:10 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Patch for landing
(19.11 KB, patch)
2019-07-24 21:11 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Ross Kirsling
Comment 1
2019-07-23 22:29:02 PDT
Created
attachment 374762
[details]
Patch
Ross Kirsling
Comment 2
2019-07-23 23:25:39 PDT
Created
attachment 374765
[details]
Patch
Maciej Stachowiak
Comment 3
2019-07-24 00:14:18 PDT
Comment on
attachment 374765
[details]
Patch Feature flag, please! (Looks like at least a compile-time flag should be practical, and maybe even runtime if it wasn't too much of a perf hit).
Ross Kirsling
Comment 4
2019-07-24 00:17:50 PDT
(In reply to Maciej Stachowiak from
comment #3
)
> Comment on
attachment 374765
[details]
> Patch > > Feature flag, please! (Looks like at least a compile-time flag should be > practical, and maybe even runtime if it wasn't too much of a perf hit).
Will do!
Ross Kirsling
Comment 5
2019-07-24 09:47:01 PDT
Created
attachment 374782
[details]
Patch
Ross Kirsling
Comment 6
2019-07-24 09:48:40 PDT
Comment on
attachment 374782
[details]
Patch I believe it should suffice to simply refuse to lex the token -- let me know if we'd like to do this differently. (I'll probably also want to update this flag to be useNullishAwareOperators in the ?. patch but there's no need to jump the gun here.)
Ross Kirsling
Comment 7
2019-07-24 12:50:47 PDT
Created
attachment 374795
[details]
Patch
Caio Lima
Comment 8
2019-07-24 13:48:16 PDT
Comment on
attachment 374795
[details]
Patch LGTM overall. I think it would be good have tests covering short circuiting.
Ross Kirsling
Comment 9
2019-07-24 15:08:57 PDT
Created
attachment 374817
[details]
Patch
Ross Kirsling
Comment 10
2019-07-24 15:09:57 PDT
(In reply to Caio Lima from
comment #8
)
> Comment on
attachment 374795
[details]
> Patch > > LGTM overall. I think it would be good have tests covering short circuiting.
Good call! Added these as well as a makeMasquerader case to cover the document.all behavior.
Darin Adler
Comment 11
2019-07-24 18:46:17 PDT
Comment on
attachment 374817
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=374817&action=review
> Source/JavaScriptCore/parser/ASTBuilder.h:1397 > + return new (m_parserArena) CoalesceNode(location, lhs.first, rhs.first);
What’s the pro and con for extending LogicalOpNode instead adding a new class?
> Source/JavaScriptCore/parser/ResultType.h:184 > + if (op1.definitelyIsBoolean() || op1.definitelyIsNumber() || op1.definitelyIsString() || op1.definitelyIsBigInt())
Could this be "definitelyIsNotNull" instead? I think we could possibly correctly cover more cases and have more maintainable code that way.
Ross Kirsling
Comment 12
2019-07-24 19:19:35 PDT
Thanks for the review! (In reply to Darin Adler from
comment #11
)
> > Source/JavaScriptCore/parser/ASTBuilder.h:1397 > > + return new (m_parserArena) CoalesceNode(location, lhs.first, rhs.first); > > What’s the pro and con for extending LogicalOpNode instead adding a new > class?
Hmm, we could do that given how similar the emitBytecode implementation is, but the emitBytecodeInConditionContext would not apply (I guess we'd just early out to emitBytecode) and we'd still call a different ResultType function. I personally thought the separation might be clearer, but do you have a preference?
> > Source/JavaScriptCore/parser/ResultType.h:184 > > + if (op1.definitelyIsBoolean() || op1.definitelyIsNumber() || op1.definitelyIsString() || op1.definitelyIsBigInt()) > > Could this be "definitelyIsNotNull" instead? I think we could possibly > correctly cover more cases and have more maintainable code that way.
We could do definitelyIsNotNullOrUndefined by checking against TypeMaybeNull and TypeMaybeOther, sure.
Ross Kirsling
Comment 13
2019-07-24 20:52:18 PDT
Created
attachment 374860
[details]
Patch for landing
Ross Kirsling
Comment 14
2019-07-24 21:10:28 PDT
Created
attachment 374863
[details]
Patch for landing
Ross Kirsling
Comment 15
2019-07-24 21:11:09 PDT
Created
attachment 374864
[details]
Patch for landing
WebKit Commit Bot
Comment 16
2019-07-25 00:50:52 PDT
Comment on
attachment 374864
[details]
Patch for landing Clearing flags on attachment: 374864 Committed
r247819
: <
https://trac.webkit.org/changeset/247819
>
WebKit Commit Bot
Comment 17
2019-07-25 00:50:54 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 18
2019-07-25 00:51:18 PDT
<
rdar://problem/53531846
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug