Attached please find a followup patch that fixes issues you have raised for
CPPADDR_NULL_PTR for other macros in JNIUtil.h, so that they become
consistent.
[[[
JavaHL: Make handling of expr and whitespace after ret_val parameters
consistent accross macros
[ in subversion/bindings/javahl/native ]
* JNIUtil.h
(SVN_JNI_NULL_PTR_EX): parenthesize expr for safety
(SVN_JNI_NULL_PTR_EX, SVN_JNI_ERR, POP_AND_RETURN): eliminate unnecessary
whitespace after ret_val
]]]
Regards,
Vladimir
On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gst...@gmail.com> wrote:
> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
> <v...@hitechman.com> wrote:
> > Patch 01 - Introduce macro
> >
> > [[[
> > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
> > checking C++ pointer extracted from the java object
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * JNIUtil.h
> > (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
> > exception if necessary
> > ]]]
>
> Replying to just this patch. The second patch seems pretty mechanical.
> And we're only looking at minor nits.
>
> (sorry, but the patch doesn't inline into this response, so let's just
> be flexible here...)
>
>
> The macro argument substitutions need to be parenthesized for safety.
> So it would be: (expr) == NULL, and it would be: return (ret_val);
>
> Next bit: the indentation in the diff seems to be off. Are there TAB
> characters in there? the JNIUTIL:: and the return lines have different
> indents in the patch that I'm looking at. That is either sloppy SPC
> character indents, or a TAB is present.
>
> Lastly, there is an extra space character before the ";" in the return
> statement. That should be eliminated.
>
>
> Fix the above three problems, and I'm +1 for you to commit just patch #1.
>
> I have not reviewed #2, but the first patch seems reasonable to
> simplify (as done in #2). I also await others to comment on the
> applicability of patch #2.
>
> I do seem to recall that C++ tried to do away with the preprocessor.
> It would be nice to follow that ideal, but looking at this macro... I
> have no idea how to map it into "proper, non-CPP concepts". If you
> know, that would be better. Otherwise... meh. CPP is just fine with me
> (and screw the C++ academic purity).
>
> Cheers,
> -g
>
Index: subversion/bindings/javahl/native/JNIUtil.h
===================================================================
--- subversion/bindings/javahl/native/JNIUtil.h (revision 1344562)
+++ subversion/bindings/javahl/native/JNIUtil.h (working copy)
@@ -214,9 +214,9 @@
*/
#define SVN_JNI_NULL_PTR_EX(expr, str, ret_val) \
- if (expr == NULL) { \
+ if ((expr) == NULL) { \
JNIUtil::throwNullPointerException(str); \
- return ret_val ; \
+ return ret_val; \
}
/**
@@ -235,7 +235,7 @@
svn_error_t *svn_jni_err__temp = (expr); \
if (svn_jni_err__temp != SVN_NO_ERROR) { \
JNIUtil::handleSVNError(svn_jni_err__temp); \
- return ret_val ; \
+ return ret_val; \
} \
} while (0)
@@ -251,7 +251,7 @@
do \
{ \
env->PopLocalFrame(NULL); \
- return ret_val ; \
+ return ret_val; \
} \
while (0)
