Bug 211256 - [JSC] intlBooleanOption should return TriState instead of taking an out param
Summary: [JSC] intlBooleanOption should return TriState instead of taking an out param
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-30 14:42 PDT by Ross Kirsling
Modified: 2020-04-30 18:14 PDT (History)
9 users (show)

See Also:


Attachments
Patch (15.30 KB, patch)
2020-04-30 14:50 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (10.44 KB, patch)
2020-04-30 17:14 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (10.16 KB, patch)
2020-04-30 17:52 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-04-30 14:42:14 PDT
[JSC] intlBooleanOption should return Optional<bool> instead of taking an out param
Comment 1 Ross Kirsling 2020-04-30 14:50:09 PDT
Created attachment 398095 [details]
Patch
Comment 2 Ross Kirsling 2020-04-30 14:53:33 PDT
Comment on attachment 398095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398095&action=review

> Source/JavaScriptCore/runtime/IntlCollator.cpp:182
> -    JSValue options = optionsValue;
> -    if (!optionsValue.isUndefined()) {
> +    JSObject* options;
> +    if (optionsValue.isUndefined())
> +        options = constructEmptyObject(vm, globalObject->nullPrototypeObjectStructure());
> +    else {

The spec does say to construct an empty object here, but since every option will be undefined anyway, we technically could just keep a null JSObject*. Not sure if there's a preferred style for this.
Comment 3 Darin Adler 2020-04-30 15:02:25 PDT
Comment on attachment 398095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398095&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] intlBooleanOption should return Optional<bool> instead of taking an out param

Sam Weinig has advised us to not use Optional<bool> because it’s too easy to get wrong, with a subtle distinction between !x and !*x / !x.value() for example. I am OK with it personally, though.

>> Source/JavaScriptCore/runtime/IntlCollator.cpp:182
>> +    else {
> 
> The spec does say to construct an empty object here, but since every option will be undefined anyway, we technically could just keep a null JSObject*. Not sure if there's a preferred style for this.

I think a nullptr for JSObject is better: less pressure on the garbage collector, smaller code size. Would we have to add a lot of null checks? Another option would be to use a pointer to a known empty object.
Comment 4 Yusuke Suzuki 2020-04-30 15:30:26 PDT
Comment on attachment 398095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398095&action=review

>>> Source/JavaScriptCore/runtime/IntlCollator.cpp:182
>>> +    else {
>> 
>> The spec does say to construct an empty object here, but since every option will be undefined anyway, we technically could just keep a null JSObject*. Not sure if there's a preferred style for this.
> 
> I think a nullptr for JSObject is better: less pressure on the garbage collector, smaller code size. Would we have to add a lot of null checks? Another option would be to use a pointer to a known empty object.

Isn't it observable if user sets `Object.prototype.someOptionName` getter? If it is observable, we need to create an object here.
Comment 5 Ross Kirsling 2020-04-30 15:43:47 PDT
(In reply to Yusuke Suzuki from comment #4)
> Isn't it observable if user sets `Object.prototype.someOptionName` getter?
> If it is observable, we need to create an object here.

This particular case won't be observable since we're meant to create an object with null prototype.
Comment 6 Ross Kirsling 2020-04-30 16:18:20 PDT
I guess the question here then is whether this is an improvement:

> -    if (options.isUndefined())
> +    if (!options)
>          return fallback;
> -    JSObject* opts = options.toObject(globalObject);
> -    RETURN_IF_EXCEPTION(scope, String());
> -
> -    JSValue value = opts->get(globalObject, property);
> +    JSValue value = options->get(globalObject, property);

Given that current JSC code style tends to assume that JSObject* params are non-null when not otherwise specified.

Perhaps I should just leave everything other than intlBooleanOption alone after all.
Comment 7 Ross Kirsling 2020-04-30 16:20:36 PDT
(In reply to Ross Kirsling from comment #6)
> Perhaps I should just leave everything other than intlBooleanOption alone
> after all.

* everything other than the Optional<bool> change, I mean.
Comment 8 Ross Kirsling 2020-04-30 17:09:49 PDT
Based on Slack discussion with Mark and Yusuke, I'm going to change this to use WTF::TriState and then rename MixedTriState to IndeterminateTriState (following Boost) in a separate patch.
Comment 9 Ross Kirsling 2020-04-30 17:14:42 PDT
Created attachment 398125 [details]
Patch

Asking for re-review just to be certain.
Comment 10 Mark Lam 2020-04-30 17:23:04 PDT
Comment on attachment 398125 [details]
Patch

r=me
Comment 11 Ross Kirsling 2020-04-30 17:52:32 PDT
Created attachment 398129 [details]
Patch for landing
Comment 12 EWS 2020-04-30 18:13:07 PDT
Committed r260976: <https://trac.webkit.org/changeset/260976>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398129 [details].
Comment 13 Radar WebKit Bug Importer 2020-04-30 18:14:14 PDT
<rdar://problem/62689888>