[Webkit-unassigned] [Bug 147604] [INTL] Implement Collator Compare Functions
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 17 13:23:02 PST 2015
https://bugs.webkit.org/show_bug.cgi?id=147604
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #267556|review? |review+
Flags| |
--- Comment #29 from Darin Adler <darin at apple.com> ---
Comment on attachment 267556
--> https://bugs.webkit.org/attachment.cgi?id=267556
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=267556&action=review
Looks like Windows is still failing to build :-(
Looks OK once Windows builds.
> 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();
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;
> 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.
> 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?
--
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/f3c89da7/attachment.html>
More information about the webkit-unassigned
mailing list