You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vladimir Berezniker <vm...@hitechman.com> on 2012/05/31 06:43:44 UTC
[PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
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
]]]
Patch 02 - Change existing code to use the new macro
[[[
JavaHL: Migrate existing code to the CPPADDR_NULL_PTR macro to reduce amount
of duplicate code
[ in subversion/bindings/javahl/native ]
* org_apache_subversion_javahl_SVNClient.cpp
(Java_org_apache_subversion_javahl_SVNClient_dispose,
Java_org_apache_subversion_javahl_SVNClient_getAdminDirectoryName,
Java_org_apache_subversion_javahl_SVNClient_isAdminDirectory,
Java_org_apache_subversion_javahl_SVNClient_getLastPath,
Java_org_apache_subversion_javahl_SVNClient_username,
Java_org_apache_subversion_javahl_SVNClient_password,
Java_org_apache_subversion_javahl_SVNClient_setPrompt,
Java_org_apache_subversion_javahl_SVNClient_logMessages,
Java_org_apache_subversion_javahl_SVNClient_checkout,
Java_org_apache_subversion_javahl_SVNClient_remove,
Java_org_apache_subversion_javahl_SVNClient_revert,
Java_org_apache_subversion_javahl_SVNClient_add,
Java_org_apache_subversion_javahl_SVNClient_update,
Java_org_apache_subversion_javahl_SVNClient_commit,
Java_org_apache_subversion_javahl_SVNClient_copy,
Java_org_apache_subversion_javahl_SVNClient_move,
Java_org_apache_subversion_javahl_SVNClient_mkdir,
Java_org_apache_subversion_javahl_SVNClient_cleanup,
Java_org_apache_subversion_javahl_SVNClient_resolve,
Java_org_apache_subversion_javahl_SVNClient_doExport,
Java_org_apache_subversion_javahl_SVNClient_doSwitch,
Java_org_apache_subversion_javahl_SVNClient_doImport,
Java_org_apache_subversion_javahl_SVNClient_merge__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2ZLorg_apache_subversion_javahl_types_Depth_2ZZZ,
Java_org_apache_subversion_javahl_SVNClient_suggestMergeSources,
Java_org_apache_subversion_javahl_SVNClient_merge__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_util_List_2Ljava_lang_String_2ZLorg_apache_subversion_javahl_types_Depth_2ZZZ,
Java_org_apache_subversion_javahl_SVNClient_mergeReintegrate,
Java_org_apache_subversion_javahl_SVNClient_properties,
Java_org_apache_subversion_javahl_SVNClient_propertySetRemote,
Java_org_apache_subversion_javahl_SVNClient_propertySetLocal,
Java_org_apache_subversion_javahl_SVNClient_revProperty,
Java_org_apache_subversion_javahl_SVNClient_revProperties,
Java_org_apache_subversion_javahl_SVNClient_setRevProperty,
Java_org_apache_subversion_javahl_SVNClient_propertyGet,
Java_org_apache_subversion_javahl_SVNClient_getMergeinfo,
Java_org_apache_subversion_javahl_SVNClient_getMergeinfoLog,
Java_org_apache_subversion_javahl_SVNClient_diff__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Ljava_io_OutputStream_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZZZZZZ,
Java_org_apache_subversion_javahl_SVNClient_diff__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Ljava_io_OutputStream_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZZZZZZ,
Java_org_apache_subversion_javahl_SVNClient_diffSummarize__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZLorg_apache_subversion_javahl_callback_DiffSummaryCallback_2,
Java_org_apache_subversion_javahl_SVNClient_diffSummarize__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZLorg_apache_subversion_javahl_callback_DiffSummaryCallback_2,
Java_org_apache_subversion_javahl_SVNClient_streamFileContent,
Java_org_apache_subversion_javahl_SVNClient_getVersionInfo,
Java_org_apache_subversion_javahl_SVNClient_upgrade,
Java_org_apache_subversion_javahl_SVNClient_relocate,
Java_org_apache_subversion_javahl_SVNClient_blame,
Java_org_apache_subversion_javahl_SVNClient_setConfigDirectory,
Java_org_apache_subversion_javahl_SVNClient_getConfigDirectory,
Java_org_apache_subversion_javahl_SVNClient_addToChangelist,
Java_org_apache_subversion_javahl_SVNClient_removeFromChangelists,
Java_org_apache_subversion_javahl_SVNClient_getChangelists,
Java_org_apache_subversion_javahl_SVNClient_lock,
Java_org_apache_subversion_javahl_SVNClient_unlock,
Java_org_apache_subversion_javahl_SVNClient_info2,
Java_org_apache_subversion_javahl_SVNClient_patch):
Switched to CPPADDR_NULL_PTR
* org_apache_subversion_javahl_SVNRepos.cpp,
(Java_org_apache_subversion_javahl_SVNRepos_dispose,
Java_org_apache_subversion_javahl_SVNRepos_create,
Java_org_apache_subversion_javahl_SVNRepos_deltify,
Java_org_apache_subversion_javahl_SVNRepos_dump,
Java_org_apache_subversion_javahl_SVNRepos_hotcopy,
Java_org_apache_subversion_javahl_SVNRepos_listDBLogs,
Java_org_apache_subversion_javahl_SVNRepos_listUnusedDBLogs,
Java_org_apache_subversion_javahl_SVNRepos_load,
Java_org_apache_subversion_javahl_SVNRepos_lstxns,
Java_org_apache_subversion_javahl_SVNRepos_recover,
Java_org_apache_subversion_javahl_SVNRepos_rmtxns,
Java_org_apache_subversion_javahl_SVNRepos_setRevProp,
Java_org_apache_subversion_javahl_SVNRepos_verify,
Java_org_apache_subversion_javahl_SVNRepos_lslocks,
Java_org_apache_subversion_javahl_SVNRepos_rmlocks,
Java_org_apache_subversion_javahl_SVNRepos_upgrade,
Java_org_apache_subversion_javahl_SVNRepos_pack,
Java_org_apache_subversion_javahl_SVNRepos_cancelOperation):
Switched to CPPADDR_NULL_PTR
]]]
Regards,
Vladimir
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Thu, May 31, 2012 at 2:35 AM, Greg Stein <gs...@gmail.com> wrote:
> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
> <vm...@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).
Like most things, C++ tried to reinvent the preprocessor and ended up
with a half-baked solution which only partially replaces the thing it
was intended to fix. We have several similar helper macros in the
JavaHL C++ layer which do are similar in checking for error conditions
(SVN_JNI_NULL_PTR_EX, for example) and possibly raising an exception.
I don't know if the proposed macro is for a common situation, but
there is precedent.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Greg Stein <gs...@gmail.com>.
+1
On May 31, 2012 11:25 PM, "Vladimir Berezniker" <vm...@hitechman.com> wrote:
> I applied the patch to the javahl-ra branch instead of the trunk by
> mistake, is
> it ok if a cherry pick it onto trunk?
>
> Sorry about that,
>
> Vladimir
>
> On Thu, May 31, 2012 at 3:37 AM, Greg Stein <gs...@gmail.com> wrote:
>
>> On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gs...@gmail.com> wrote:
>> >...
>> > Fix the above three problems, and I'm +1 for you to commit just patch
>> #1.
>>
>> To clarify: I trust you to make the three changes, and you're free to
>> commit with my +1. I don't need another round of review.
>>
>> (if something is missing, then we can fix in a second commit)
>>
>> Thanks,
>> -g
>>
>
>
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Vladimir Berezniker <vm...@hitechman.com>.
I applied the patch to the javahl-ra branch instead of the trunk by
mistake, is
it ok if a cherry pick it onto trunk?
Sorry about that,
Vladimir
On Thu, May 31, 2012 at 3:37 AM, Greg Stein <gs...@gmail.com> wrote:
> On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gs...@gmail.com> wrote:
> >...
> > Fix the above three problems, and I'm +1 for you to commit just patch #1.
>
> To clarify: I trust you to make the three changes, and you're free to
> commit with my +1. I don't need another round of review.
>
> (if something is missing, then we can fix in a second commit)
>
> Thanks,
> -g
>
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Greg Stein <gs...@gmail.com>.
On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gs...@gmail.com> wrote:
>...
> Fix the above three problems, and I'm +1 for you to commit just patch #1.
To clarify: I trust you to make the three changes, and you're free to
commit with my +1. I don't need another round of review.
(if something is missing, then we can fix in a second commit)
Thanks,
-g
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Greg Stein <gs...@gmail.com>.
On May 31, 2012 11:07 PM, "Vladimir Berezniker" <vm...@hitechman.com> wrote:
> On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gs...@gmail.com> wrote:
>>...
>> (sorry, but the patch doesn't inline into this response, so let's just
>> be flexible here...)
>>
> Sorry, I cannot figure out how to get gmail to inline attachments, I'll
setup
> IMAP client when I have a spare moment to to figure out which one will
> work for me.
Oh, attachments are best because they don't get munged in various ways. It
was just that I was on my tablet (now, too), and it is near impossible to
get the attachments into the body.
*shrug*
>> The macro argument substitutions need to be parenthesized for safety.
>> So it would be: (expr) == NULL, and it would be: return (ret_val);
>
>
> will take care of (expr). Same cannot be done for ret_val because it is
> valid for the ret_val to be empty when used inside void methods.
Saw that, yeah.
>...
> I am a java guy, with a some C/C++ understanding, afraid I won't be of
> much help in this department. Most of the macro is repeat of the existing
> SVN_JNI_NULL_PTR_EX with different exception being thrown.
No worries. Just curious.
Cheers,
-g
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Vladimir Berezniker <vm...@hitechman.com>.
On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gs...@gmail.com> wrote:
> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
> <vm...@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...)
>
> Sorry, I cannot figure out how to get gmail to inline attachments, I'll
setup
IMAP client when I have a spare moment to to figure out which one will
work for me.
>
> The macro argument substitutions need to be parenthesized for safety.
> So it would be: (expr) == NULL, and it would be: return (ret_val);
>
will take care of (expr). Same cannot be done for ret_val because it is
valid for the ret_val to be empty when used inside void methods.
>
> 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.
>
I managed to sneak a single TAB in there, fixed now.
>
> 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.
>
Will do.
>
> 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.
I am a java guy, with a some C/C++ understanding, afraid I won't be of
much help in this department. Most of the macro is repeat of the existing
SVN_JNI_NULL_PTR_EX with different exception being thrown.
> Otherwise... meh. CPP is just fine with me
> (and screw the C++ academic purity).
>
> Cheers,
> -g
>
Thank you for the feedback,
Vladimir
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Greg Stein <gs...@gmail.com>.
+1 for trunk. Thanks.
On May 31, 2012 11:28 PM, "Vladimir Berezniker" <vm...@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 <gs...@gmail.com> wrote:
>
>> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
>> <vm...@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
>>
>
>
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Vladimir Berezniker <vm...@hitechman.com>.
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 <gs...@gmail.com> wrote:
> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
> <vm...@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
>
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Thu, May 31, 2012 at 4:36 AM, Julian Foad <ju...@btopenworld.com> wrote:
>
>
>
>
> ----- Original Message -----
>> From: Greg Stein <gs...@gmail.com>
>> To: Vladimir Berezniker <vm...@hitechman.com>
>> Cc: dev@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
>> <vm...@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/
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
Posted by Julian Foad <ju...@btopenworld.com>.
----- Original Message -----
> From: Greg Stein <gs...@gmail.com>
> To: Vladimir Berezniker <vm...@hitechman.com>
> Cc: dev@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
> <vm...@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
>
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++
pointer extracted from the java object
Posted by Greg Stein <gs...@gmail.com>.
On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
<vm...@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