https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/ibm/icu/charset/CharsetCallback.java File main/classes/charset/src/com/ibm/icu/charset/CharsetCallback.java (right): https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/ibm/icu/charset/CharsetCallback.java#newcode3 main/classes/charset/src/com/ibm/icu/charset/CharsetCallback.java:3: * Copyright (C) 2006-2011, International Business Machines Corporation and ...
10 years, 4 months ago
(2014-07-30 20:48:56 UTC)
#4
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
File main/classes/charset/src/com/ibm/icu/charset/CharsetCallback.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
main/classes/charset/src/com/ibm/icu/charset/CharsetCallback.java:3: * Copyright
(C) 2006-2011, International Business Machines Corporation and *
On 2014/07/30 01:03:51, roubert (google) wrote:
> Year not updated.
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
File main/classes/charset/src/com/ibm/icu/charset/CharsetMBCS.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
main/classes/charset/src/com/ibm/icu/charset/CharsetMBCS.java:220: String
itemName = myName + "." + UConverterSharedData.DATA_TYPE;
On 2014/07/30 01:03:51, roubert (google) wrote:
> I think that having this myName + "." + UConverterSharedData.DATA_TYPE logic
> before if (loader != null) like it was before, so that that piece of logic is
> explicitly shared between all branches of these chained if-statements, was
> slightly more logical and easy to maintain.
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
main/classes/charset/src/com/ibm/icu/charset/CharsetMBCS.java:466: sbcsIndex =
new char[SBCS_FAST_LIMIT>>6];
On 2014/07/30 01:03:51, roubert (google) wrote:
> Are these lines of code now supposed to be commented out? If they are, then I
> suggest instead commenting them out Eclipse style with a // prefix on each
line
> (instead of including them before the */ end marker), to make that crystal
> clear.
>
> (As a bonus, it then also becomes trivial in Eclipse to toggle them being
> commented-out or not.)
Done. I would prefer Eclipse putting the // at the least common indent, rather
than at the start of the line, but I left it.
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
main/classes/charset/src/com/ibm/icu/charset/CharsetMBCS.java:486:
asciiRoundtrips&=~(1<<(i>>2));
On 2014/07/30 01:03:51, roubert (google) wrote:
> While you're editing here anyway: Could you add some strategically placed
spaces
> to increase the chance of anyone being able to read this statement correctly?
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
File main/classes/charset/src/com/ibm/icu/charset/UConverterDataReader.java
(right):
https://codereview.appspot.com/121870043/diff/1/main/classes/charset/src/com/...
main/classes/charset/src/com/ibm/icu/charset/UConverterDataReader.java:606:
boolean isFormatVersionAtLeast_6_1() {
On 2014/07/30 01:03:51, roubert (google) wrote:
> Wouldn't it be better to name these functions something along the lines of
> formatHasUnicodeMask() so that it at the call site is directly obvious why it
is
> being called any why it is safe to assume that there is a unicodeMask if the
> function returned true?
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/collate/src/com/...
File main/classes/collate/src/com/ibm/icu/impl/coll/CollationRoot.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/collate/src/com/...
main/classes/collate/src/com/ibm/icu/impl/coll/CollationRoot.java:50: t = t2;
On 2014/07/30 01:03:49, roubert (google) wrote:
> What's the purpose of having the temporary variable t2, instead of assigning
> directly to t and passing t to read()?
It's the easiest way to keep t=null until after an exception may be thrown.
> If the purpose is something non-obvious,
> then it might be worth commenting on.
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/impl/ICUBinary.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:133: return base +
bytes.getInt(base + 4 + 4 + index * 8);
On 2014/07/30 01:03:50, roubert (google) wrote:
> Symbolic names for all these integer constants would make this much easier to
> read.
They can also make an expression so long that it's less readable. I beefed up
the comments instead.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:233: private static String
simpleTrim(String s, int start, int limit) {
On 2014/07/30 01:03:50, roubert (google) wrote:
> Does this function add any real value over just calling s.substring().trim()
> instead?
I wrote my own splitting code to avoid the regex pattern compilation in
String.split(sep) [I wish there was a simple by-character split] and the
StringTokenizer "is discouraged in new code". I might be over-optimizing.
And I wrote my own trim() because
a) I wanted to avoid the substring(), and
b) I thought String.trim() does something more complicated, with Unicode
White_Space. Now I see that it trims [\u0000-\u0020] which is weird but standard
and fast.
Replacing with .substring().trim().
There should really be an efficient, character-based splitAndTrim()...
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:322: * @throws
MissingResourceException if required==true and the resource could not be found
On 2014/07/30 01:03:50, roubert (google) wrote:
> There is no throws-declaration at the function itself and no parameter named
> required. This looks like copy-paste error.
No, it passes required=true into the function it calls.
I copied this naming & calling from the ICUData.getRequiredStream() functions.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:336: * @throws
MissingResourceException if required==true and the resource could not be found
On 2014/07/30 01:03:50, roubert (google) wrote:
> See comment above.
ditto
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:337: public static
ByteBuffer getRequiredData(ClassLoader loader, String resourceName,
On 2014/07/30 01:03:50, roubert (google) wrote:
> See comment in CharsetMBCS.java about commenting out code.
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:354: * @throws
MissingResourceException if required==true and the resource could not be found
On 2014/07/30 01:03:50, roubert (google) wrote:
> There is no throws-declaration at the function itself. Shouldn't there be one?
> It's a RuntimeException, but in ICUResourceBundle.java there are throws
> MissingResourceException declarations.
I don't like throws declarations. Java code throws a variety of exceptions for
lots of reasons, even when not declared. When a caller needs to know about an
interesting one, we can document it. Required declarations for
non-RuntimeExceptions are evil. I would prefer not to add not-required throws
declarations either.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:401: file.close();
On 2014/07/30 01:03:50, roubert (google) wrote:
> FileChannel.map() can throw exceptions, so shouldn't the close() call be in a
> finally-clause?
It seems unlikely that map() fails when we were able to get a channel.
The trouble with file.close() inside finally is that it would need its own
try-catch for the @#$% IOException it may throw.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:401: file.close();
On 2014/07/30 01:03:50, roubert (google) wrote:
> You should also add a comment explaining that this will close the channel as
> well. Few people will know that, and then it looks as if this code forgets to
> close the channel.
That comment is already there, but I clarified it a bit.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:506: return 32;
On 2014/07/30 01:03:50, roubert (google) wrote:
> I suggest adding an assert dos.size() == 32; primarily to make it clear to the
> reader why 32 is a good number to return here.
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/impl/ICUData.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUData.java:143:
@SuppressWarnings("unused")
On 2014/07/30 01:03:49, roubert (google) wrote:
> A comment that this will be used if logging is activated would be helpful to
> prevent deletion of this code by mistake.
Done.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/impl/ICUData.java:175: * Returns null if the
resource could not be found.
On 2014/07/30 01:03:50, roubert (google) wrote:
> Is there any reason to not use @return Javadoc syntax for these comments
> instead?
@return should document the normal value and the exception; I just wanted to
clarify the error behavior.
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/RBBIDataWrapper.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/RBBIDataWrapper.java:91: //ivate static
final int RESERVED_2 = 6;
On 2014/07/30 01:03:50, roubert (google) wrote:
> http://en.wikipedia.org/wiki/Ivate
>
> Vacation! ;-)
LOL
Nice, it's winter but it's still very warm.
http://in.weather.com/weather/weekend/Ivat%C3%A9+PR+Brazil+BRXX2222:1:BRhttps://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/RBBIRuleBuilder.java (right):
https://codereview.appspot.com/121870043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/RBBIRuleBuilder.java:2: // Copyright
(C) 2002-2009, International Business Machines Corporation and others.
On 2014/07/30 01:03:51, roubert (google) wrote:
> Year not updated.
Done.
Issue 121870043: ICU ticket #10939: ICU4J optionally read data from .dat and .res
(Closed)
Created 10 years, 4 months ago by markus.icu
Modified 10 years, 4 months ago
Reviewers: roubert (google)
Base URL: http://source.icu-project.org/repos/icu/icu4j/branches/markus/mapdata/
Comments: 38
