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...) 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
