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