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

Roger Riggs Roger.Riggs at Oracle.com
Mon Dec 5 21:44:39 UTC 2016


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 changethe actual setting, otherwise a new instance with*the requested autoFlush *is returned*.*

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



More information about the core-libs-dev mailing list