On Thu, May 31, 2012 at 3:35 AM, Greg Stein <gst...@gmail.com> wrote:
> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker > <v...@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
