<html>
    <head>
      <base href="https://bugs.webkit.org/" />
    </head>
    <body><span class="vcard"><a class="email" href="mailto:darin&#64;apple.com" title="Darin Adler &lt;darin&#64;apple.com&gt;"> <span class="fn">Darin Adler</span></a>
</span> changed
              <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - [INTL] Implement the caseFirst option for Intl.Collator"
   href="https://bugs.webkit.org/show_bug.cgi?id=158188">bug 158188</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 #280100 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 the caseFirst option for Intl.Collator"
   href="https://bugs.webkit.org/show_bug.cgi?id=158188#c5">Comment # 5</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - [INTL] Implement the caseFirst option for Intl.Collator"
   href="https://bugs.webkit.org/show_bug.cgi?id=158188">bug 158188</a>
              from <span class="vcard"><a class="email" href="mailto:darin&#64;apple.com" title="Darin Adler &lt;darin&#64;apple.com&gt;"> <span class="fn">Darin Adler</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=280100&amp;action=diff" name="attach_280100" title="Patch">attachment 280100</a> <a href="attachment.cgi?id=280100&amp;action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=280100&amp;action=review">https://bugs.webkit.org/attachment.cgi?id=280100&amp;action=review</a>

<span class="quote">&gt; Source/JavaScriptCore/runtime/IntlCollator.cpp:289
&gt;      m_numeric = (result.get(ASCIILiteral(&quot;kn&quot;)) == &quot;true&quot;);</span >

I’d like how this reads better without the parentheses.

<span class="quote">&gt; Source/JavaScriptCore/runtime/IntlCollator.cpp:290
&gt; +    const String&amp; caseFirst = result.get(ASCIILiteral(&quot;kf&quot;));</span >

As with the cases below, I think it’s nicer to have the translation from string to CaseFirst be in a helper function. It can be inlined if we like.

<span class="quote">&gt; Source/JavaScriptCore/runtime/IntlCollator.cpp:296
&gt; +        ASSERT(caseFirst == &quot;false&quot;);</span >

Why can we assert this? What prevents invalid values?

<span class="quote">&gt; Source/JavaScriptCore/runtime/IntlCollator.cpp:389
&gt; +    UColAttributeValue caseFirst;
&gt; +    switch (m_caseFirst) {
&gt; +    case CaseFirst::Upper:
&gt; +        caseFirst = UCOL_UPPER_FIRST;
&gt; +        break;
&gt; +    case CaseFirst::Lower:
&gt; +        caseFirst = UCOL_LOWER_FIRST;
&gt; +        break;
&gt; +    case CaseFirst::False:
&gt; +        caseFirst = UCOL_OFF;
&gt; +        break;
&gt; +    default:
&gt; +        ASSERT_NOT_REACHED();
&gt; +    }</span >

It’s a better idiom to put this kind of translation into a helper function that takes a CaseFirst and returns a UColAttributeValue. Then we can use return inside the switch cases, so we can put the ASSERT_NOT_REACHED outside the switch instead of in a default case, and the compiler will give us a warning if we ever leave a case out. It also makes the main code flow easier to read and helps us spot if we made a mistake.

This would be similar to the caseFirstString function. It can be inlined if we like.

Only trick might be using the UColAttributeValue type in the class header without including ICU headers.

<span class="quote">&gt; Source/JavaScriptCore/runtime/IntlCollator.cpp:451
&gt; +const char* IntlCollator::caseFirstString(CaseFirst caseFirst)</span >

I think we should probably mark this function inline.

<span class="quote">&gt; Source/JavaScriptCore/runtime/IntlCollator.cpp:462
&gt; +    return nullptr;</span >

I think we should return &quot;false&quot; here instead of nullptr.</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>