Issue 308460043: Greek Uppercasing

Issue 308460043: Greek Uppercasing (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
7 years, 7 months ago by andy.heninger

Modified:
6 years, 8 months ago

Reviewers:
andy.heninger, markus.icu

Base URL:
svn+ssh://source.icu-project.org/repos/icu/icu/trunk/

Visibility:
Public.

More Reviews

Patch Set 3 : ICU4J 39252 #

Total comments: 2

Created: 7 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+7 lines, -5 lines)			Patch
M	coverage-exclusion.txt	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	main/classes/core/src/com/ibm/icu/impl/CaseMap.java	View	1 2	3 chunks	+4 lines, -4 lines	0 comments	Download
M	main/classes/core/src/com/ibm/icu/impl/UCaseProps.java	View	1 2	1 chunk	+1 line, -1 line	2 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

andy.heninger

A few very minor comments. https://codereview.appspot.com/308460043/diff/1/source/common/ucase.h File source/common/ucase.h (right): https://codereview.appspot.com/308460043/diff/1/source/common/ucase.h#newcode162 source/common/ucase.h:162: /** @return same as ...

7 years, 7 months ago (2016-09-23 22:49:31 UTC) #3

markus.icu

7 years, 7 months ago (2016-09-27 21:35:03 UTC) #4

https://codereview.appspot.com/308460043/diff/1/source/common/ucase.h
File source/common/ucase.h (right):

https://codereview.appspot.com/308460043/diff/1/source/common/ucase.h#newcode162
source/common/ucase.h:162: /** @return same as ucase_getType() and set bit 2 if
c is case-ignorable */
On 2016/09/23 22:49:30, andy.heninger wrote:
> Is bit 2 UCASE_IGNORABLE?
> Maybe say that instead of bit 2.

Done.

https://codereview.appspot.com/308460043/diff/1/source/common/ucasemap.cpp
File source/common/ucasemap.cpp (right):

https://codereview.appspot.com/308460043/diff/1/source/common/ucasemap.cpp#ne...
source/common/ucasemap.cpp:419: // Keep this consistent with the UTF-16 version
in ustrcase.cpp and the Java version in CaseMap.java.
On 2016/09/23 22:49:30, andy.heninger wrote:
> The Java version does not have a corresponding keep-consistent comment.

Done.

https://codereview.appspot.com/308460043/diff/1/source/common/ucasemap.cpp#ne...
source/common/ucasemap.cpp:500: destIndex=appendUChar(dest, destIndex,
destCapacity, (UChar)upper);
On 2016/09/23 22:49:30, andy.heninger wrote:
> Already fixed for int overflow checking, just not visible here.

Yes, that was in a later ticket+changeset.

https://codereview.appspot.com/308460043/diff/40001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/UCaseProps.java (right):

https://codereview.appspot.com/308460043/diff/40001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/UCaseProps.java:1346: public static final
int IGNORABLE=      4;
On 2016/09/23 22:49:30, andy.heninger wrote:
> Why public? Package scope would make it visible to CaseMap.GreekUpper, which
> appears to be the only place using it.

It was public because my prototype was in a different package. Changed to
package-visible. Thanks for noticing. Of course, public would not be horrible in
an impl-package class, but we don't need that right now.

Expand All Messages | Collapse All Messages