On Mon, Mar 26, 2018 at 9:53 AM, Alan Bateman <alan.bate...@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
