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

[Jennifer Godinez]

Hi Neil,

Looks good to me.  Please use the created bug ID for the comment.  No 
need to send new webrev for that though.

Jennifer

Phil Race wrote:
> 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
>>
>