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

[bugzilla-daemon at webkit.org]

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>