On 14/04/2012 19:29, Xueming Shen wrote:
Hi Martin,
Thanks for taking on this one. Here are my comments after first scan
(1) Upon return, the position of the ByteBuffer should not always be
updated to the "limit". It should depend
on the number of bytes really compressed/de-compressed. Like the
buffer at ReadableByteChannel.read(buffer).
(2) The implementation of the native buffer version and non-buffer
version probably can share most of their
code in a separated method
(3) The input part will be tough. I was struggling with if we should
have a totally separated subclass, like
DeflaterBuffer/InfalterBuffer (or BufferDefalter/Inflater) to only
handle everything in ByteBuffer with
methods handles buffer input and output, throw "not supported
operation" for those "byte[]" methods.
Otherwise you will have to put something in the specification to
mandate the behavior of mixed bytebuff
and byte[] scenario. I'm not sure which way is more appropriate though.
-Sherman
Sherman - I'll assume you'll sponsor Martin once he's signed up to
contribute.
I didn't go through the patch in detail but I did notice that the buffer
position should be pos + number of bytes written to buffer. Also the
ArrayIndexOutOfBoundsException doesn't look right as it's not testable
and means the invariant defined by Buffer is violated. In other places
its an assert. A test case will also be needed, I didn't see that in the
patch.
I didn't set new setInput methods in the patch but I agree that would
require much more consideration.
-Alan
