On Jan 4 2011, at 07:03 , Chris Hegarty wrote:
> On 23/12/2010 20:29, Mike Duigou wrote:
>> Feedback is on webrev.02
>> http://cr.openjdk.java.net/~chegar/7000511/webrev.02/webrev/ :
>>
>> * PrintStream
>>
>> - .flush(), close(), most println() methods synchronize on this for their
>> entire implementation. They could just be made synchronized methods.
>
> I think there are pros and cons of doing this, maybe best left to another CR
> which could look into this separately?
Just an observation. Yes, definitely separately, if at all.
>
>> - The javadoc for append(CharSequence,int,int) for the csq==null condition
>> seems misleading as start and end are considered.
>
> I'm not sure what the original intent of this was, but from what I can see
> the javadoc appears to be clear and the implementation looks like it behaves
> correctly.
Rereading it in a new year I'm not as bothered by it. The wording could better
reflect that substitution of the string "null" for csq=null is the only special
handling for csq=null.
- "An invocation ..." could perhaps be improved to:
This method is equivalent to calling append(CharSequence) with the subsequence
of {...@code csq}, or the string "null" if {...@code csq} is {...@code null},
defined by {...@code start} and {...@code end}. <pre>...</pre>
- @param csq : "If <tt>csq</tt> is ..." sentence to read:
A {...@code null} value is equivalent to passing the string "null".
I feel more strongly about the later change than the former (my alternative
wording doesn't seem much better to me).
>
>> * Formatter
>>
>> - The zero params constructor could just call this((Apppendable) null). The
>> advantage is that the StringBuilder is only constructed in one place.
>>
>> - The Formatter(Locale) constructor could just call this(l, (Apppendable)
>> null). Same reason.
>
> Actually, I think it is clearer to have the StringBuilder created as is. It
> makes it more obvious than having to trace through another constructor. But
> it you feel strongly I can make the changes.
Personal preference only. (It's also easy to follow the trail with Netbeans)
>
> -Chris.
>
>>
>>
>> On Dec 14 2010, at 07:07 , Chris Hegarty wrote:
>>
>>> Failing java.io.PrintStream, java.io.PrintWriter, java.util.Scanner, and
>>> java.util.Formatter multi-arg constructors that take a java.io.File or
>>> String filename (as well as one or more additional args) opens a
>>> FileIn/OutputStream to the given File/filename. If one of the other given
>>> args causes the constructor to fail ( null or unsupported charset for
>>> example ) the FileIn/OutputStream is never closed, and the application does
>>> not have a reference to it. You rely on the finalizer to close the stream.
>>>
>>> This is most serious on Windows because you cannot remove a file if there
>>> is an open handle to it.
>>>
>>> I also cleaned up an existing regression test that fails in samevm mode
>>> (partly) because of this. And removed another excluded test from the list
>>> since its bug was fixed some time ago.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~chegar/7000511/webrev.00/webrev/
>>>
>>> -Chris.
>>
