<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604#c31">Comment # 31</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604">bug 147604</a>
from <span class="vcard"><a class="email" href="mailto:sukolsak@gmail.com" title="Sukolsak Sakshuwong <sukolsak@gmail.com>"> <span class="fn">Sukolsak Sakshuwong</span></a>
</span></b>
<pre>Oops, sorry. I accidentally clicked Save Changes. I will fix the linking issue on Windows.
(In reply to <a href="show_bug.cgi?id=147604#c29">comment #29</a>)
<span class="quote">> 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;</span >
Will do.
<span class="quote">> > 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.</span >
Will do.
<span class="quote">> > 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?</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>