<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #267556 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<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#c29">Comment # 29</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:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=267556&action=diff" name="attach_267556" title="Patch">attachment 267556</a> <a href="attachment.cgi?id=267556&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=267556&action=review">https://bugs.webkit.org/attachment.cgi?id=267556&action=review</a>
Looks like Windows is still failing to build :-(
Looks OK once Windows builds.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollator.cpp:304
> + if (sensitivityString.isNull()) {
> + // a. If u is "sort", then let s be "variant".
> + // b. Else
> + // i. Let dataLocale be the value of r.[[dataLocale]].
> + // ii. Let dataLocaleData be Get(localeData, dataLocale).
> + // iii. Let s be Get(dataLocaleData, "sensitivity").
> + // 10.2.3 "[[searchLocaleData]][locale] must have a sensitivity property with a String value equal to "base", "accent", "case", or "variant" for all locale values."
> + sensitivityString = ASCIILiteral("variant");
> + }
> + // 27. Set collator.[[sensitivity]] to s.
> + if (sensitivityString == "base")
> + m_sensitivity = Sensitivity::Base;
> + else if (sensitivityString == "accent")
> + m_sensitivity = Sensitivity::Accent;
> + else if (sensitivityString == "case")
> + m_sensitivity = Sensitivity::Case;
> + else if (sensitivityString == "variant")
> + m_sensitivity = Sensitivity::Variant;
> + else
> + ASSERT_NOT_REACHED();</span >
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 class="quote">> Source/JavaScriptCore/runtime/IntlCollator.cpp:325
> +void IntlCollator::createCollator(ExecState& state)
> +{</span >
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 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);</span >
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?</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>