Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object

On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
<vmpn_at_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
Received on 2012-05-31 09:36:17 CEST