Hello,

 I have updated the fix according to the comment:

 * split the variable declaration and the macro

 * re-wrote the macro in suggested manner

 Please note that the macro forces an error exit if we
 failed to get the io handler, so there seems to be no risk
 of NPE.

 Please take a look to updated fix:
 http://cr.openjdk.java.net/~bae/8020983/8/webrev.02/

Thanks,
Andrew

On 7/30/2013 4:18 AM, Jim Graham wrote:
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





Reply via email to