Code review request for 4206909 : adding Z_SYNC_FLUSH support to deflaters
Xueming Shen
Xueming.Shen at Sun.COM
Wed Oct 21 17:50:19 UTC 2009
Thanks Alan, the webrev has been updated according.
http://cr.openjdk.java.net/~sherman/4206909/webrev
sherman
Alan Bateman wrote:
> Xueming Shen wrote:
>> Martin, Alan.
>>
>> Finally got the CCC approval. Here is the final webrev
>>
>> http://cr.openjdk.java.net/~sherman/4206909/webrev
>>
>> The only difference compared to the webrev you guys reviewed last
>> time at
>> http://cr.openjdk.java.net/~sherman/zipflush/webrev is to use the
>> Google copyright
>> in test case InflateIn_DeflateOut.java
>>
>> Thanks,
>> Sherman
>>
> Good to see this one coming near to the finish. The changes mostly
> look good to me and the test looks good (thanks Martin). One comment
> is that DeflaterOutputStream.syncFlush be final. Other than that, I
> only have a few minor nits:
>
> 1. In DeflaterOutputStream.flush() it would be nicer if the test was:
> if (syncFlush && !def.finished()) { ... }
> That might the line wrap in the middle of the while expression.
>
> 2. Should you use @throws instead of @exception? (I haven't seen the
> latter in several years).
>
> 3. Do you need the blank line between the @see and @since tags in
> Deflater's constants? (seems locally inconsistent).
>
> -Alan.
>
More information about the core-libs-dev
mailing list