[Webkit-unassigned] [Bug 147602] [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147602

--- Comment #12 from Geoffrey Garen <ggaren at apple.com> ---
> > These need initializers.
> > 
> > > Source/JavaScriptCore/runtime/IntlNumberFormat.h:91
> > > +    bool m_useGrouping;
> > 
> > And this.
> 
> The only functions that read these attributes are
> Intl.NumberFormat.prototype.resolvedOptions and (in the upcoming patch)
> Intl.NumberFormat.prototype.format. They always check that the NumberFormat
> object has been initialized. If not, they initialize it. These attributes
> are always set in the initialization. So, I think it's fine to not have
> initializers for these. What do you think?

I think you're right that the code will be correct without initializers.

Still, it is good idiomatic practice to initialize from the start so that readers of this code do not need to think about issues like this.

The only case where I would leave out an initializer is a case where performance is super-critical and removing the initial write saves a large fraction of time. I don't think this is one of those cases.

> Related question: The only time resolvedOptions() is called, but the
> NumberFormat object has not been initialized, is when we call
> "Intl.NumberFormat.prototype.resolvedOptions()". The spec says that "The
> Intl.NumberFormat prototype object is itself an Intl.NumberFormat instance
> ... whose internal slots are set as if it had been constructed by the
> expression Construct(%NumberFormat%)." But I don't want to populate the
> internal slots of the prototype eagerly, because Intl.NumberFormat.prototype
> is created when JSGlobalObject is created. Is it fine to do lazy
> initialization like this?

I think lazy initialization is OK. I'm not sure exactly how I would engineer it in this case.

> 
> > > Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:73
> > > +    Vector<String> keyLocaleData({
> > 
> > Can we use std::array here instead? We don't intend to grow or shrink this
> > data structure.
> 
> The localeData() function is passed to the resolveLocale() function, which
> expects a function pointer of type Vector<String> (*localeData)(const
> String&, const String&). I can't change the return type to std::array
> because vectors of different size are also passed to it.

Okeedokee.

> 
> > > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:156
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumIntegerDigits"), jsNumber(nf->minimumIntegerDigits()));
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumFractionDigits"), jsNumber(nf->minimumFractionDigits()));
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "maximumFractionDigits"), jsNumber(nf->maximumFractionDigits()));
> > > +    if (nf->minimumSignificantDigits())
> > > +        options->putDirect(vm, Identifier::fromString(&vm, "minimumSignificantDigits"), jsNumber(nf->minimumSignificantDigits()));
> > > +    if (nf->maximumSignificantDigits())
> > > +        options->putDirect(vm, Identifier::fromString(&vm, "maximumSignificantDigits"), jsNumber(nf->maximumSignificantDigits()));
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "useGrouping"), jsBoolean(nf->useGrouping()));
> > 
> > I think we want these to be CommonIdentifiers so we don't have to keep
> > allocating and hashing them.
> 
> There are only two places that use these identifiers: the constructor and
> prototype.resolvedOptions(), which probably won't be called very often.
> Should I still add these to CommonIdentifiers?

I guess it's a close call. Let's leave it as strings for now.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151103/54df492c/attachment.html>