Request for Review and Sponsor needed: JDK-8167648: java.io.PrintWriter should have PrintWriter((String|File), Charset) constructors

Patrick Reinhart patrick at reini.net
Mon Dec 5 22:29:40 UTC 2016


> Am 05.12.2016 um 22:44 schrieb Roger Riggs <Roger.Riggs at Oracle.com>:
> 
> Hi Patrick,
> 
> On 11/30/2016 4:47 PM, Patrick Reinhart wrote:
>> ...
>>> A few comments on the webrev:
>>> 
>>> - 359: The withAutoFlush javadoc should be more explicit about when a new vs the same
>>>   PrintWriter is returned.  The ‚activates‘ verb doesn't convey any sense about the instance that is returned.
>>  359      * Activates {@code printf}, or {@code format} methods to automatically 
>>  360      * flush the output buffer after writing their data.
>> 
>> Would the following be better:
>> 
>> Returns the same instance if {@code autoFlush} does not change the 
>> actual setting, otherwise a new instance with changed behavior is returned
>> 
> my $.02 version
> Returns the same instance if {@code autoFlush} does not change the 
> actual setting, otherwise a new instance with the requested autoFlush is returned.
Sounds good to me.  If there are no other suggestions I will use that one..

>>> - 375:  Can this use the new private constructor that will handle psOut.
>> Here I can not get hold on the encoding at this point or have I missed something here?
> True, not easily, it is available as a String from OutputStreamWriter.getEncoding() but it would suffer from 
> having to lookup it by name again.

Right, even the Charset is actually available within the StreamEncoder implementation but not provided to the outside world.

Also the OutputStreamWriter is in wrapped in a BufferedWriter and would be needed to be extracted from there again in the first place. 
>> 
>>> -320, etc. The @since should be 1 or 2 digits to match the version scheme
>> It seems, that I’m still not in the new version scheme ;-) - should be 9 - I will change that, the same for 343
>> 
>>> - no tests for new PrintWriter(OutputStream <non-null>, Charset)
>> I will also add that
>> 
>>> - From the test file name 'FailngConstructors", its not clear that's the right place
>>>   for the positive tests of the withAutoFlush methods.
>> I will move that out to a new Test as soon we are more clear about the other points
> ok
> 
> Thanks, Roger
>> 
>>> That's all I have time for at the moment,
>>> 
>>> Regards, Roger
>>> 
>>> 
>>> On 11/29/2016 4:15 PM, Patrick Reinhart wrote:
>>>> Does anyone sponsor this fix?
>>>> 
>>>> http://cr.openjdk.java.net/~reinhapa/reviews/8167648/webrev.00 <http://cr.openjdk.java.net/~reinhapa/reviews/8167648/webrev.00>
>>>> 
>>>> -Patrick
> 



More information about the core-libs-dev mailing list