ICU ticket #12410: CaseMap with Edits

C++ UTF-16 & Java

[markus.icu, 2017-02-14 00:50:52]

Yoshito: Could you do the whole review? Or do you want Andy to take the C++
part?

Others FYI. This is based on a few rounds of API design.

https://codereview.appspot.com/311670043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/text/CaseMap.java (right):

https://codereview.appspot.com/311670043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/text/CaseMap.java:61: public static Fold
fold() { return Fold.DEFAULT; }
I wonder if we should call this method  foldCase()  after all. The "case" is
redundant with the class name, but I stumbled over the difference with the C++
CaseMap::foldCase() method and with UCharacter.foldCase() etc.

[yoshito, 2017-02-14 23:14:12]

On 2017/02/14 00:50:52, markus.icu wrote:
> Yoshito: Could you do the whole review? Or do you want Andy to take the C++
> part?

I'll review Java part. I appreciate if Andy can take the C/C++ part.

>
https://codereview.appspot.com/311670043/diff/1/icu4j/main/classes/core/src/c...
> icu4j/main/classes/core/src/com/ibm/icu/text/CaseMap.java:61: public static
Fold
> fold() { return Fold.DEFAULT; }
> I wonder if we should call this method  foldCase()  after all. The "case" is
> redundant with the class name, but I stumbled over the difference with the C++
> CaseMap::foldCase() method and with UCharacter.foldCase() etc.

UCharacter uses foldCase() as the method name. But at the same time,
toUppderCase() / toLowerCase() and so on.
Internal ucase_* APIs in ICU4C use function names without 'case' -
ucase_tolower, ucase_toupper, ucase_fold...

I looks proposed C++ method names are not consistent. CaseMap::toLower,
CaseMap::toUpper, CaseMap::toTitle... but CaseMap::foldCase.
I think if we use the term Case in the method names, others should also follow
the convention (toUpperCase, toTitleCase...)

Why can't you update C++ method name to ::folld() instead of updating the method
name in Java?

[markus.icu, 2017-02-15 00:37:58]

On 2017/02/14 23:14:12, yoshito wrote:
> On 2017/02/14 00:50:52, markus.icu wrote:
>
https://codereview.appspot.com/311670043/diff/1/icu4j/main/classes/core/src/c...
> > icu4j/main/classes/core/src/com/ibm/icu/text/CaseMap.java:61: public static
> Fold
> > fold() { return Fold.DEFAULT; }
> > I wonder if we should call this method  foldCase()  after all. The "case" is
> > redundant with the class name, but I stumbled over the difference with the
C++
> > CaseMap::foldCase() method and with UCharacter.foldCase() etc.
> 
> UCharacter uses foldCase() as the method name. But at the same time,
> toUppderCase() / toLowerCase() and so on.
> Internal ucase_* APIs in ICU4C use function names without 'case' -
> ucase_tolower, ucase_toupper, ucase_fold...

Public C/C++ API is mostly consistent without "case" except for foldCase().

ustring.h: u_strToLower(), u_strFoldCase()
unistr.h: toLower(), foldCase()
ucasemap.h: ucasemap_utf8ToLower(), ucasemap_utf8FoldCase()

> I looks proposed C++ method names are not consistent. CaseMap::toLower,
> CaseMap::toUpper, CaseMap::toTitle... but CaseMap::foldCase.
> I think if we use the term Case in the method names, others should also follow
> the convention (toUpperCase, toTitleCase...)
> 
> Why can't you update C++ method name to ::folld() instead of updating the
method
> name in Java?

I agree that "case" is redundant given the class name.
I had the C++ names consistent with previous C/C++ API but it can't be
consistent with those as well as with Java UCharacter.

I am ok with changing C++ to just fold() as well.

[markus.icu, 2017-02-15 00:47:38]

FYI

In Java, I renamed impl/CaseMap.java to impl/CaseMapImpl.java to avoid collision
with new text/CaseMap.java. Rietveld does not show this.  See
http://bugs.icu-project.org/trac/changeset?old=39603&old_path=branches%2Fmark...

[markus.icu, 2017-02-15 22:53:08]

C++ foldCase()->fold(), UTF-8 new-file properties

[yoshito, 2017-02-16 14:51:16]

I finished reviewing the Java implementation.
The code looks pretty clean and I think Java changes are ready to go.
Thanks Markus!

[andy.heninger, 2017-02-17 05:59:54]

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
File icu4c/source/common/edits.cpp (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
icu4c/source/common/edits.cpp:145: UBool Edits::growArray() {
Too bad you can't use MaybeStackArray. I think it actually works as a member of
a class, but probably not when it leaks into a public header.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
icu4c/source/common/edits.cpp:151: return FALSE;
Not related to this code specifically, but maybe we could start using the
language's bool, true and false. Except for members of structs with strict
portable layout requirements, it should be fine, and would make our language
dialect less weird.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
icu4c/source/common/edits.cpp:162: uint16_t *newArray = (uint16_t
*)uprv_malloc((size_t)newCapacity * 2);
uprv_realloc?
Maybe not, since it doesn't help when the source is the stack array.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
File icu4c/source/common/unicode/casemap.h (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:45: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
UChars?

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:53: * @return The length of the result
string, if successful - or in case of a buffer overflow,
This description seems awkward. Maybe
The length of the result string. If the length is greater than the destCapacity
a buffer overflow error will be reported.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:78: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
UChars

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:123: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
UChars

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:163: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
UChars

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
File icu4c/source/common/unicode/edits.h (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/edits.h:120: * The iterator state after this search
is undefined
Is this the same as for next()? Undefined if function returns false.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/edits.h:180: int32_t oldLength_, newLength_;
Why do some member variable have underscores, and some not?

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/ustrc...
File icu4c/source/common/ustrcase.cpp (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/ustrc...
icu4c/source/common/ustrcase.cpp:48: edits->copyErrorTo(errorCode);
Edits can also generate an internal U_BUFFER_OVERFLOW_ERROR, which might be
confusing to calling code that assumes that they need to grow their buffer.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/ustrc...
icu4c/source/common/ustrcase.cpp:943: UBool change;
Slightly simpler 
    UBool change = TRUE;
    if (edits) {
        // Find out ...

[markus.icu, 2017-02-17 18:31:22]

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
File icu4c/source/common/edits.cpp (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
icu4c/source/common/edits.cpp:145: UBool Edits::growArray() {
On 2017/02/17 05:59:53, andy.heninger wrote:
> Too bad you can't use MaybeStackArray. I think it actually works as a member
of
> a class, but probably not when it leaks into a public header.

Acknowledged.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
icu4c/source/common/edits.cpp:151: return FALSE;
On 2017/02/17 05:59:53, andy.heninger wrote:
> Not related to this code specifically, but maybe we could start using the
> language's bool, true and false. Except for members of structs with strict
> portable layout requirements, it should be fine, and would make our language
> dialect less weird.

Maybe. We should discuss our strategy in the team first. I won't use the
built-in types and constants until we have consensus.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/edits...
icu4c/source/common/edits.cpp:162: uint16_t *newArray = (uint16_t
*)uprv_malloc((size_t)newCapacity * 2);
On 2017/02/17 05:59:53, andy.heninger wrote:
> uprv_realloc?
> Maybe not, since it doesn't help when the source is the stack array.

Acknowledged.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
File icu4c/source/common/unicode/casemap.h (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:45: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
On 2017/02/17 05:59:53, andy.heninger wrote:
> UChars?

Done.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:53: * @return The length of the result
string, if successful - or in case of a buffer overflow,
On 2017/02/17 05:59:53, andy.heninger wrote:
> This description seems awkward. Maybe
> The length of the result string. If the length is greater than the
destCapacity
> a buffer overflow error will be reported.

Done.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:78: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
On 2017/02/17 05:59:53, andy.heninger wrote:
> UChars

Done.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:123: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
On 2017/02/17 05:59:53, andy.heninger wrote:
> UChars

Done.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/casemap.h:163: * @param destCapacity The size of the
buffer (number of bytes). If it is 0, then
On 2017/02/17 05:59:53, andy.heninger wrote:
> UChars

Done.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
File icu4c/source/common/unicode/edits.h (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/edits.h:120: * The iterator state after this search
is undefined
On 2017/02/17 05:59:53, andy.heninger wrote:
> Is this the same as for next()? Undefined if function returns false.

We discussed this and agreed to leave it undefined. (next()->FALSE sets the
iterator to an empty no-change.)

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/unico...
icu4c/source/common/unicode/edits.h:180: int32_t oldLength_, newLength_;
On 2017/02/17 05:59:53, andy.heninger wrote:
> Why do some member variable have underscores, and some not?

I prefer without underscores, except when the simple name wants to be the name
of the getter or of a parameter.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/ustrc...
File icu4c/source/common/ustrcase.cpp (right):

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/ustrc...
icu4c/source/common/ustrcase.cpp:48: edits->copyErrorTo(errorCode);
On 2017/02/17 05:59:53, andy.heninger wrote:
> Edits can also generate an internal U_BUFFER_OVERFLOW_ERROR, which might be
> confusing to calling code that assumes that they need to grow their buffer.

Thanks!
Edits now set a U_INDEX_OUTOFBOUNDS_ERROR instead.

https://codereview.appspot.com/311670043/diff/20001/icu4c/source/common/ustrc...
icu4c/source/common/ustrcase.cpp:943: UBool change;
On 2017/02/17 05:59:53, andy.heninger wrote:
> Slightly simpler 
>     UBool change = TRUE;
>     if (edits) {
>         // Find out ...

Done.

[markus.icu, 2017-02-17 18:43:57]

changes for review feedback

[markus.icu, 2017-02-17 18:56:40]

changes for review feedback (for real now)

[andy.heninger, 2017-02-17 19:10:43]

On 2017/02/17 18:56:40, markus.icu wrote:
> changes for review feedback (for real now)

LGTM