ticket:10944: Add ByteBuffer support for StringPrep.

ticket:10944: Add ByteBuffer support for StringPrep.

Also adds the ICUBinary.getVersionByteArrayFromCompactInt() function.

R=markus.icu@gmail.com

Committed: http://bugs.icu-project.org/trac/changeset/36037

[markus.icu, 2014-07-11 18:37:11]

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/StringPrepDataReader.java (right):

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/StringPrepDataReader.java:32: * @param
bytes ICU uprop.dat file buffer
ICU StringPrep data file buffer

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/StringPrepDataReader.java:78:
(byte)unicodeVersion.getMajor(),
It might be a little better to store the unicodeVersion as an int and just turn
it into a byte array here. You could create a method in ICUBinary for that,
similar to the
    VersionInfo getVersionInfoFromCompactInt(int version)
that's already there.
For example,
    byte[] getVersionByteArrayFromCompactInt(int version)

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/text/StringPrep.java (right):

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/text/StringPrep.java:285: public
StringPrep(ByteBuffer bytes) throws IOException{
Don't just add public API without proposal, see my longer comment in another
review. For now, please just move this code into the existing constructor.
Again, you could add a TODO for adding a ByteBuffer overload.

https://codereview.appspot.com/106580043/diff/60001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/stringprep/NFS4StringPrep.java
(right):

https://codereview.appspot.com/106580043/diff/60001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/stringprep/NFS4StringPrep.java:42:
nfscsiFile.close();
Please move the declarations of all of these streams to before the "try", and
add a "finally" where all of them are closed.

[roubert (google), 2014-07-14 15:04:33]

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/StringPrepDataReader.java (right):

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/StringPrepDataReader.java:32: * @param
bytes ICU uprop.dat file buffer
On 2014/07/11 18:37:11, markus.icu wrote:
> ICU StringPrep data file buffer

Done.

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/StringPrepDataReader.java:78:
(byte)unicodeVersion.getMajor(),
On 2014/07/11 18:37:11, markus.icu wrote:
> It might be a little better to store the unicodeVersion as an int and just
turn
> it into a byte array here. You could create a method in ICUBinary for that,
> similar to the
>     VersionInfo getVersionInfoFromCompactInt(int version)
> that's already there.
> For example,
>     byte[] getVersionByteArrayFromCompactInt(int version)

Done.

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/text/StringPrep.java (right):

https://codereview.appspot.com/106580043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/text/StringPrep.java:285: public
StringPrep(ByteBuffer bytes) throws IOException{
On 2014/07/11 18:37:11, markus.icu wrote:
> Don't just add public API without proposal, see my longer comment in another
> review. For now, please just move this code into the existing constructor.
> Again, you could add a TODO for adding a ByteBuffer overload.

Done.

https://codereview.appspot.com/106580043/diff/60001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/stringprep/NFS4StringPrep.java
(right):

https://codereview.appspot.com/106580043/diff/60001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/stringprep/NFS4StringPrep.java:42:
nfscsiFile.close();
On 2014/07/11 18:37:11, markus.icu wrote:
> Please move the declarations of all of these streams to before the "try", and
> add a "finally" where all of them are closed.

Instead, I reverted all changes to this file, as they only are relevant when
adding a StringPrep constructor taking ByteBuffer.

[markus.icu, 2014-07-15 17:02:27]

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
File main/classes/core/src/com/ibm/icu/impl/ICUBinary.java (right):

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:270: (byte)((version >>
16) & 0xff),
Remove the "& 0xff" -- you are casting to a byte anyway.

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
File main/classes/core/src/com/ibm/icu/text/StringPrep.java (right):

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
main/classes/core/src/com/ibm/icu/text/StringPrep.java:289: sprepTrie = new
CharTrie(ByteBuffer.wrap(sprepBytes), null);
Wait, we already have a ByteBuffer. We should be able to read from that rather
than fetching the sprepBytes for it.

[roubert (google), 2014-07-15 19:29:16]

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
File main/classes/core/src/com/ibm/icu/impl/ICUBinary.java (right):

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:270: (byte)((version >>
16) & 0xff),
On 2014/07/15 17:02:27, markus.icu wrote:
> Remove the "& 0xff" -- you are casting to a byte anyway.

Done.

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
File main/classes/core/src/com/ibm/icu/text/StringPrep.java (right):

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
main/classes/core/src/com/ibm/icu/text/StringPrep.java:289: sprepTrie = new
CharTrie(ByteBuffer.wrap(sprepBytes), null);
On 2014/07/15 17:02:27, markus.icu wrote:
> Wait, we already have a ByteBuffer. We should be able to read from that rather
> than fetching the sprepBytes for it.

Done.

[markus.icu, 2014-07-15 19:46:38]

LGTM

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
File main/classes/core/src/com/ibm/icu/text/StringPrep.java (right):

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
main/classes/core/src/com/ibm/icu/text/StringPrep.java:213: private static final
int INDEX_TRIE_SIZE                  =  0; /* number of bytes in normalization
trie */
I would keep this line for documentation, just comment it out.

[roubert (google), 2014-07-15 19:59:11]

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
File main/classes/core/src/com/ibm/icu/text/StringPrep.java (right):

https://codereview.appspot.com/106580043/diff/120001/main/classes/core/src/co...
main/classes/core/src/com/ibm/icu/text/StringPrep.java:213: private static final
int INDEX_TRIE_SIZE                  =  0; /* number of bytes in normalization
trie */
On 2014/07/15 19:46:38, markus.icu wrote:
> I would keep this line for documentation, just comment it out.

Done.

[markus.icu, 2014-07-15 20:03:23]

LGTM++

[roubert (google), 2014-07-15 20:49:52]

Committed patchset #9 manually as r36037 (presubmit successful).