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
