[OpenJDK 2D-Dev] Request for review: [NEW BUG] Printer spoolers ignore result from spool process

[Phil Race]

Looks fine to me. Jennifer, can you also review ?

I've filed 7105640: Unix printing does not check the result of exec'd 
lpr/lp command

The reason I commented on the formatting is that the indentation seemed 
to have
so much variation I thought you were using tabs. Aesthetically I'd have 
done it a
little differently, even if the guidelines allow what you have here. But 
I am not
going to quibble over it.

-phil.

On 10/27/2011 9:53 AM, Neil Richards wrote:
> On Thu, 2011-10-20 at 18:41 -0700, Phil Race wrote:
>>> PS you may want to look at your indentation. It appears inconsistent.
>>> Always use exactly 4 spaces for indentation.
>>>
>>> -phil.
>>>
>>> On 10/20/2011 12:20 PM, Phil Race wrote:
>>>> Neil,
>>>>
>>>> You are throwing an IOException and we already have the code to catch
>>>> that and re-throw as the documented printer exception type, and also
>>>> the event should
>>>> get notified to any listeners.  So far so good.
>>>>
>>>> But you are throwing the IOException before calling
>>>> spoolFile.delete() so we'll leak
>>>> the spool file.
>>>> Arguably that could happen anyway if there were an Exception but that
>>>> was
>>>> probably not occurring previously.
>>>>
>>>> I suggest either to move the spoolfile.delete() to *before* the
>>>> handleError call
>>>> or into a finally { } block.
>>>>
>>>> -phil.
> Hi Phil,
> Thanks for taking a look at my suggestion.
>
> I agree that the existing code does run the risk of leaking the spool
> file (in exceptional circumstances), and that my change exacerbates that
> risk.
>
> I've modified the suggested fix (in both places) to move the delete()
> call to a finally block.
> (This also meant moving the initial check for the spool file's existence
> to before the 'try' block.)
> I've uploaded the resulting webrev for review [1].
>
> ----
>
> For coding conventions, the current OpenJDK Developer's Guide [2] still
> points to the "Code Conventions for the Java Programming Language" [3].
>
>
> In that document, in section 7.2 "Compound Statements", it gives the
> following guidance on the placement of braces:
>
>                * The enclosed statements should be indented one more
>                  level than the compound statement.
>                * The opening brace should be at the end of the line that
>                  begins the compound statement; the closing brace should
>                  begin a line and be indented to the beginning of the
>                  compound statement.
>                * Braces are used around all statements, even singletons,
>                  when they are part of a control structure, such as a
>                  if-else or for statement. This makes it easier to add
>                  statements without accidentally introducing bugs due to
>                  forgetting to add braces.
>
> I believe the code in my suggested fix consistently follows this
> guidance in this respect.
>
>
> Also in that document, in section 4.2 "Wrapping lines", it gives the
> following guidance on the breaking of an expression across lines:
>
>          When an expression will not fit on a single line, break it
>          according to these general principles:
>                * Break after a comma.
>                * Break before an operator.
>                * Prefer higher-level breaks to lower-level breaks.
>                * Align the new line with the beginning of the expression
>                  at the same level on the previous line.
>                * If the above rules lead to confusing code or to code
>                  that’s squished up against the right margin, just indent
>                  8 spaces instead.
>
> Note that the 'try ( ... ) {' statements associated with automatic
> resource management count as a single expression, and therefore are
> subject to these guidelines.
>
> "Aligning the new line with the beginning of the expression at the same
> level on the previous line" (4th bullet) would, in these cases, lead to
> "confusing code" (5th bullet) as it would be indented almost identically
> to the code within the try block (5 spaces vs. 4).
> Therefore, I chose to follow the guidelines of the 5th bullet and "just
> indent 8 spaces instead".
>
> To avoid the parameters to 'handleProcessFailure()' from being "squished
> up against the right margin", I similarly chose to follow the guidelines
> of the 5th bullet.
>
> So I believe the code in my suggested fix consistently follows the
> convention's guidelines in this respect.
>
>
> (On all other lines, four spaces are used as the unit of indentation, as
> specified in section 4 of the same document).
>
>
> If there is some later documented and agreed coding convention that I
> should be following in preference to the one I (and the OpenJDK's
> Developer's Guide) refer to, I'd be most grateful if you could point me
> in its direction.
>
> Thanks, Neil
>
> [1] http://cr.openjdk.java.net/~ngmr/ojdk-201/webrev.03/
> [2] http://openjdk.java.net/guide/codeConventions.html
> [3] http://www.oracle.com/technetwork/java/codeconventions-150003.pdf
>