[JSC] intlBooleanOption should return Optional<bool> instead of taking an out param
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].
<rdar://problem/62689888>