On 23/03/2018 19:17, David Lloyd wrote:
All fixed. You're right, the catch blocks consolidated fairly easily
and it looks better. I've attached the latest version of the patch.
I went through the APIs and javadoc changes attached to your last mail.
This version addresses most of the points I brought up a few weeks ago
and the API generally looks good. The only methods that I'm not sure
about is the setDictionary variants as I don't see a bit need for these.
Deflater.setInput currently has "The given buffer's position will be
updated ...". I think could be clearer by saying that the buffer
position s advanced by the deflate operations up to its limit. This
will make it more consistent with the wording in the deflate methods. I
also wonder if the parameter should be renamed to "input" or "source".
The deflate methods talk about "remaining space" which is a bit
inconsistent the buffer APIs where it uses "bytes remaining". I think we
should try to keep this as consistent as possible.
Also the usage advice, "Make sure the buffer's remaining space ..."
should probably be moved to an @apiNote (this goes for the existing
deflate methods too).
I don't have cycles just now to go through all the implementation but I
think Sherman is doing that. It will need careful review to avoid being
abused to attack memory outside of the buffer. I did check the use of
position() and limit() to calculate the remaining and these need
correct. Style wide then we should try to keep thing consistent with the
existing code as possible (most of the "final" usages are a distraction
and aren't needed for example).
-Alan
