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

On Thu, May 31, 2012 at 4:36 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
>
>
>
>
> ----- 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.

We use this same trick for other macros, such as
SVN_JNI_NULL_PTR_EX(). I don't know if it is standardized, but we've
been using it for years. We do have to omit the parenthesis around
the return value in the macro definition, as "return ();" is not valid
syntax.

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/