Summary: | [JSC] intlBooleanOption should return TriState instead of taking an out param | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||||
Component: | New Bugs | Assignee: | Ross Kirsling <ross.kirsling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ross Kirsling
2020-04-30 14:42:14 PDT
Created attachment 398095 [details]
Patch
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 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 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. (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. 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.
(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. 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. Created attachment 398125 [details]
Patch
Asking for re-review just to be certain.
Comment on attachment 398125 [details]
Patch
r=me
Created attachment 398129 [details]
Patch for landing
Committed r260976: <https://trac.webkit.org/changeset/260976> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398129 [details]. |