----- Original Message -----
> From: Greg Stein <gstein_at_gmail.com>
> To: Vladimir Berezniker <vmpn_at_hitechman.com>
> Cc: dev_at_subversion.apache.org
> Sent: Thursday, 31 May 2012, 8:35
> Subject: 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);
I notice the second patch relies on being able to pass an empty (whitespace-only) second argument in order to generate "return;" in the macro, so putting parentheses there wouldn't work. Actually I didn't know it was possible to pass an empty (or, rather, whitespace-only) argument to a macro, but apparently it is. Is it standardized? If so, this seems fine to me, to use the argument without adding parentheses around it.
- Julian
> 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 11:36:49 CEST