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
