[JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater

[David Lloyd]

On Mon, Mar 26, 2018 at 9:53 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
> 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.

I think they may have use in some cases e.g. reading a dictionary from
a file, and generally in code which prefers byte buffers over arrays
(completely outside of the direct/heap buffer question), and they were
easy enough to implement.  But I'm okay dropping these methods if that
is what is desired.

> 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.

OK.

> I also wonder if the parameter should be renamed to "input" or "source".

I was going for some kind of consistency with the array variants (that
is, name the parameter what it is), though they simply use "b" for
their parameter name so that interpretation might be a stretch.
Should I update them all?

> 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.

OK.

> 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'm not 100% familiar with the new JavaDoc categories (ok I'm 0%
familiar), but the JEP for these says "This category consists of
commentary, rationale, or examples pertaining to the API.".  But this
feels more like specification to me since it is "specifications that
apply to all valid implementations, including preconditions,
postconditions, etc.".  Which is to say, if you don't provide enough
remaining space in the output buffer, you will have incorrect
operation as a result.  WDYT?

> 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.

I hope this was a typo of "these seem correct" and not a typo of
"these need correcting"?

> 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).

OK I can remove them; it's been my personal convention for so many
years that I don't see them when they're there (only when they're
missing).  But removing them here is OK with me.


-- 
- DML