[OpenJDK 2D-Dev] [8] request for review: 8020983: OutOfMemoryError caused by non garbage collected JPEGImageWriter Instances

[Jim Graham]

One suggestion I'd add, though, is that the GET() macro is written so as 
to be difficult to maintain.  It both declares a variable and executes 
code which means it does not act like a declaration, nor does it act 
like a piece of code.  Also, if someone needs to create a similar type 
of macro later, they would have no ability to inject into any code that 
uses this macro.

I'd separate out the variable declaration and just have the macro deal 
with loading the value.

I also am not a fan of macros ending with an "if" statement since they 
leave an optional state as their residue.  If the following statement is 
an "else", the person using the macro may believe that the else will 
apply to their "if", but it would actually apply to the "if" in the macro:

	if (mycondition)  MACRO(foo) else bar;

In that case the "else bar;" would apply to the if inside the macro. 
And, using it with a closing semicolon would yeild a compile error:

	if (mycondition)  MACRO(foo); else bar;

would generate an exception because the else comes after a null 
statement due to the "};" that is expanded from the tail end of the 
macro body.

The style guide recommends always using braces because of this type of 
case, but I prefer to write self-protecting macros:

#define MACRO(foo) \
	do { \
            if (blah) stuff; \
         } while (0)

The semicolon that typically follows the macro invocation would turn 
this into a single statement compatible with just about any use in C code.

Additionally, I don't have my JNI handbook with me, but looking on line 
I am not convinced that the follow-on JNI methods that try to call a 
method on the (potentially null) stream can deal with a null object to 
invoke on.  They don't list NPE as one of the exceptions that they 
throw.  Some added logic to avoid invoking on a null stream may be needed...

			...jim

On 7/29/13 2:32 PM, Phil Race wrote:
> Looks fine.
>
> -phil.
>
> On 7/29/2013 2:13 PM, Andrew Brygin wrote:
>> Hello,
>>
>>  please take a look to updated fix:
>>
>>  http://cr.openjdk.java.net/~bae/8020983/8/webrev.01/
>>
>>  The name 'stream' is now eliminated.
>>
>>  I have verified that the change does not trigger any existing
>>  regression tests for imageio jpeg.
>>
>> Thanks,
>> Andrew
>>
>> On 7/29/2013 7:21 PM, Mario Torre wrote:
>>> On Mon, 2013-07-29 at 19:05 +0400, Andrew Brygin wrote:
>>>> Hi Mario,
>>>>
>>>>    thanks for the comments!
>>>>
>>>>    I agree that 'stream' and 'streamRef' are not appropriate names now,
>>>>    because these entities point to instances of writer/reader.
>>>>
>>>>    However, these entities use the writer/reader only as a holder of
>>>>    certain set of I/O routines, so probably names for these entities
>>>>    should reflect their I/O nature.
>>>>
>>>>    Would 'ioRef' and 'input'/'output' sound better than 'stream'?
>>> Hi Andrew,
>>>
>>> Those seems better defaults, yes.
>>>
>>> At first I got confused with "stream" because the struct used to hold
>>> the Input/Output stream for real. I guess what really bothers me though
>>> is the comment:
>>>
>>> jweak streamRef; // ImageInputStream or ImageOutputStream
>>>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> Which is misleading now :) so thanks for addressing this!
>>>
>>> Cheers,
>>> Mario
>>>
>>>
>>
>