Am 24.10.2012 04:56, schrieb Xueming Shen:
Thanks for the review. I hope I addressed most of the comments in the
updated webrev at
http://cr.openjdk.java.net/~sherman/4235519/webrev
Hi Sherman,
my additional comments:
I'm "confused" about the order of xxcode() and Xxcoder.
In other places e.g. charsets, we have de... before en..., which is also true
alphabetical
1026 private Base64() {}
1027 private static final int MIMELINEMAX = 76;
1028 private static final byte[] crlf = new byte[] {'\r', '\n'};
- difficult to find, please move (more) to the beginning, at least not in between inner class
definitions.
- I more would like CRLF than crlf
118 * @param lineLength the length of each output line (rounded down
119 * to nearest multiple of 4).
This could be interpreted as: the parameter must be rounded before, so maybe
better:
118 * @param lineLength the length of each output line (if not a
multiple of 4,
119 * it will be rounded down accordingly).
126 public static Encoder getEncoder(int lineLength, byte[] lineSeparator)
{
127 Objects.requireNonNull(lineSeparator);
128 return new Encoder(false, lineSeparator, lineLength / 4 * 4);
129 }
- What (should) happen(s), if lineSeparator.length == 0 ?
- Isn't one of these little faster, or at least more clear? :
lineLength -= lineLength % 4
lineLength & 0xFFFFFFFC
lineLength << 2 >> 2
Broken indentation (why at all, compared to lines 213..215?):
208 private final byte[] newline;
209 private final int linemax;
210 private final boolean isURL;
Unconventional indentation and even broken in lines 223..224 (maybe more?):
247 * @param src the byte array to encode
248 * @param dst the output byte array
249 * @return The number of bytes written to the output byte
array
250 *
251 * @throws IllegalArgumentException if {@code dst} does not
have
252 * enough space for encoding all input bytes.
More conventional:
247 * @param src the byte array to encode
248 * @param dst the output byte array
249 * @return The number of bytes written to the output byte array
250 *
251 * @throws IllegalArgumentException if {@code dst} does not have
252 * enough space for encoding all input bytes.
603 static {
604 Arrays.fill(fromBase64, -1);
605 for (int i = 0; i < Encoder.toBase64.length; i++)
606 fromBase64[Encoder.toBase64[i]] = i;
607 fromBase64['='] = -2;
608 }
This causes the inner Encoder class to be loaded, even if not needed. So maybe move the toBase64
table to the outer class.
Have you compared performance with fromBase64 as byte[] (including local 'b' in
decode() as byte)?
Retrieving the bytes by b = base64[x] then could be done without address-shift and smaller/faster
LoadB operations could be used by JIT. At least the footprint of the table would be smaller.
Missing spaces:
805 return new DecInputStream(is, isURL?fromBase64URL:fromBase64,
isMIME);
Why at all you repeat this code many times? Why not? :
631 this.base64 = isURL ? fromBase64URL : fromBase64;
Also it would at least be helpful for readers to define final, e.g.:
809 final int[] base64 = isURL ? fromBase64URL : fromBase64;
Why you mix the algorithms to check the padding? :
824 if (b == -2) { // padding byte
890 if (b == '=') {
-Ulf
