Hi Paul, thanks for reviewing the changeset, comment inlined below. On 01/20/2014 09:24 AM, Paul Sandoz wrote:
Some quick comments.
In String.toLowerCase:
- it would be nice to get rid of the pseudo goto using the "scan" labelled block.
webrev has been updated to remove the pseudo goto by checking the "first" against "len" after the loop break.
- there is no need for the "localeDependent" local variable.
I just tried to not throw away the result of this "localeDependent", which is still needed in the toXCaseEx() methods.
- you might be able to optimize by doing (could depend on the answer to the next point):
int c = (int)value[i]; int lc = Character.toLowerCase(c); if (.....) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); }
- Do you need to check ERROR for the result of toLowerCase?
2586 if (c == Character.ERROR ||
Yes, Character.toLowerCase() should never return ERROR (while the package private Character.toUpperCaseEx() will). In theory there is no need to check if the return value of Character.toUpperCase(int) > min_supplementary_code_point in our loop, because there is no bmp character returns a supplementary code point as its lower case. But since it's a data driven mapping table, there is no guarantee the unicode data table is not going to change in the "future", so I still keep the check. The webrev has been updated to use one single "if" inside the loop. I have a "single if" implementation for the toUpperCase() as well, though don't sure which one is better/faster :-) http://cr.openjdk.java.net/~sherman/8032012/ -Sherman