+1 for trunk. Thanks.
On May 31, 2012 11:28 PM, "Vladimir Berezniker" <vmpn_at_hitechman.com> wrote:
> 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 <gstein_at_gmail.com> wrote:
>
>> 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-06-01 09:58:57 CEST