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

[Neil Richards]

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

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU