[webkit-reviews] review granted: [Bug 216253] Derive jis0208 and jis0212 tables from ICU : [Attachment 408198] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 216253: Derive jis0208 and jis0212 tables from ICU
https://bugs.webkit.org/show_bug.cgi?id=216253

Attachment 408198: Patch

https://bugs.webkit.org/attachment.cgi?id=408198&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 408198
  --> https://bugs.webkit.org/attachment.cgi?id=408198
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408198&action=review

r=me assuming all the tests pass

> Source/WebCore/platform/text/EncodingTables.cpp:1064
> +	   array = new std::array<std::pair<uint16_t, UChar>, 7724>();

No need for the parens here.

> Source/WebCore/platform/text/EncodingTables.cpp:1081
> +		   ucnv_toUnicode(icuConverter.get(), &output, output + 1,
&input, input + sizeof(icuInput), nullptr, true, &error);

Are these 8836 separate calls to ucnv_toUnicode fast enough for our purposes?

> Source/WebCore/platform/text/EncodingTables.cpp:1887
> +		   ucnv_toUnicode(icuConverter.get(), &output, output + 1,
&input, input + sizeof(icuInput), nullptr, true, &error);

Ditto.

> Source/WebCore/platform/text/EncodingTables.h:47
> -template<typename CollectionType> void sortByFirst(CollectionType&);
> +template<typename CollectionType> void stableSortByFirst(CollectionType&);

I suggest keeping both around because std::sort is more efficient than
std::stable_sort when they keys are all unique.

We would use sortByFirst in any cases where the keys are unique, and it could
even do the sortedFirstsAreUnique assertion. Removing the assertion would be an
nice cleanup to the call sites.