Faulty Null-Check Suspected in ToolProvider

[Jonathan Gibbons]

I still don't see why it is necessary to specify this behavior.

-- Jon

On 2/16/19 4:45 PM, Philipp Kunz wrote:
> Hi Jon,
>
> On Sat, 2019-02-16 at 13:44 -0800, Jonathan Gibbons wrote:
>> On 2/16/19 12:20 AM, Philipp Kunz wrote:
>>> I'm also wondering about the call to flush in run(PrintStream out, 
>>> PrintStream err, String... args). It looks like the intention was to 
>>> flush the wrapping PrintWriter. That is not possible without also 
>>> flushing the underlying PrintStream. BufferedWriter.flushBuffer 
>>> would be a more sensible method to call but is not accessible. The 
>>> effect is actually, that the call to PrintWriter.flush will also 
>>> call flush of the underlying PrintStream. Should that be documented 
>>> more explicitly, for example: 
>>
>>
>> Philipp,
>>
>> I don't see that it needs to be specified.
>> It is a reasonable presumption that everything written by the tool
>> is propagated to the streams passed into the run method. How that
>> is achieved is an implementation detail.
>
> I absolutely agree so far. My point, however, was a different one.
> In addition to flushing the buffers created by run(PrintStream, 
> PrintStream, String...)'s PrintWriters to the PrintStreams passed to 
> that method, flushing the PrintWriters propagates to the underlying 
> PrintStreams and flushes them as well.
> In other words, whatever PrintStreams passed into run(PrintStream, 
> PrintStream, String...) are flushed.
>
>> If you were to modify the spec, it would at most be an implementation
>> detail, and should appear in an @implNote.
>
> I agree that it should go into an @implNote. I also suggested it that 
> way, below an existing @implNote.
>
>> -- Jon
>>
>
> Additionally, run(PrintStream, PrintStream, String...) is only a 
> default method and may be replaced in ToolProvider implementations.
> Therefore we don't know if these buffers will actually be flushed and 
> I replaced "are" with "may be" before "flushed" in the comment.
> The same consideration also applies to run(PrintWriter, PrintWriter, 
> String...).
>
>
> diff -r 31e3aa9c0c71 
> src/java.base/share/classes/java/util/spi/ToolProvider.java
> --- a/src/java.base/share/classes/java/util/spi/ToolProvider.java Sat 
> Feb 16 11:40:34 2019 +0900
> +++ b/src/java.base/share/classes/java/util/spi/ToolProvider.java Sun 
> Feb 17 01:30:30 2019 +0100
> @@ -75,6 +75,8 @@
>       * @apiNote The interpretation of the arguments will be specific to
>       * each tool.
>       *
> +     * @implNote Both {@code <mailto:{@code> out} and {@code 
> <mailto:{@code> err} streams may be flushed.
> +     *
>       * @param out a stream to which "expected" output should be written
>       *
>       * @param err a stream to which any error messages should be written
> @@ -107,6 +109,8 @@
>       * @implNote This implementation wraps the {@code <mailto:{@code> 
> out} and {@code <mailto:{@code> err}
>       * streams within {@link <mailto:{@link> PrintWriter}s, and then 
> calls
>       * {@link <mailto:{@link> #run(PrintWriter, PrintWriter, String[])}.
> +     * Both {@code <mailto:{@code> out} and {@code <mailto:{@code> 
> err} streams may be flushed before the method
> +     * returns as a side-effect of flushing the wrapping {@link 
> <mailto:{@link> PrintWriter}s.
>       *
>       * @param out a stream to which "expected" output should be written
>       *
>
> Regards,
> Philipp