[Webkit-unassigned] [Bug 147604] [INTL] Implement Collator Compare Functions

[bugzilla-daemon at webkit.org]

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

--- Comment #31 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Oops, sorry. I accidentally clicked Save Changes. I will fix the linking issue on Windows.

(In reply to comment #29)
> Seems a little silly to do this as two steps. It could just be:
> 
>     else if (sensitivityString.isNull() || sensitivityString == "variant")
>         m_sensitivity = Sensitivity::Variant;
> 
> Or even skip the ASSERT_NOT_REACHED thing and do this:
> 
>     else
>         m_sensitivity = Sensitivity::Variant;

Will do.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:325
> > +void IntlCollator::createCollator(ExecState& state)
> > +{
> 
> I think we could use an ASSERT(!m_collator) at the start of this function to
> help make it clear locally in this function that we won’t leak m_collator by
> overwriting it without closing it.

Will do.

> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:93
> > +    // 1. Let collator be the this value.
> > +    IntlCollator* collator = jsDynamicCast<IntlCollator*>(state->thisValue());
> > +
> > +    // 2. Assert: Type(collator) is Object and collator has an [[initializedCollator]] internal slot whose value is true.
> > +    ASSERT(collator);
> 
> I’m really surprised that “Assert” in the specification really means what
> ASSERT means in our code. I would have expected that an assertion in the
> specification is more about conditions that would lead to an exception as
> opposed to conditions that can’t possible be true because we are guaranteed
> the caller won’t violate certain invariants.
> 
> Is it really true that no one can call this function with a thisValue that
> is not an IntlCollator*? If so, then why do a jsDynamicCast rather than a
> static_cast?

It's true. Here's the definition of Intl.Collator.prototype.compare:

    "the phrase 'this Collator object' refers to the object that is the this value for the invocation of the function; a TypeError exception is thrown if the this value is not an object or an object that does not have an [[initializedCollator]] internal slot with value true."

    "1. Let collator be this Collator object.
    2. If collator.[[boundCompare]] is undefined, then
        a. Let F be a new built-in function object as defined in 11.3.4.
        b. The value of F’s length property is 2.
        c. Let bc be BoundFunctionCreate(F, «this value»).
        d. Set collator.[[boundCompare]] to bc.
    3. Return collator.[[boundCompare]]."

"F" in Step 2 is our IntlCollatorFuncCompare function. Since it's bound to a value that we have already checked that it is a Collator, the spec can assert that.

We do have a test case "new Intl.Collator().compare.call(5, 'a', 'b')". As expected, it does not throw.

I will change jsDynamicCast to jsCast.

-- 
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/20151217/aaba99a9/attachment.html>