Error checking might distract the main flow of code but it would be
far-fetched to call that it obfuscates the code. Ideally error checking
could have been made a function (inline if possible) instead of a macro
but since it is context sensitive in this case (have to free variables
depending on context) , doing so would obfuscate the code even more.
I tried to separate it into a function and the code looks more uglier.
-Harsha
On Tuesday 30 August 2016 09:26 AM, David Holmes wrote:
On 30/08/2016 1:47 PM, Harsha Wardhana B wrote:
Hi Dmitry,
I am not sure how using macro will help in this context. As far as I
know, macros must be sparingly used as they are error-prone, obfuscate
the code and hard to debug. Most of best coding practice for c/c++
(inluding Scott Myers Effective C++ ) advocate using macros only if it
is absolutely needed.
Macros are used extensively throughout hotspot and the JDK native
code. That isn't to say that all such macro uses are good (we have bad
code too). However macros make sense where you want to avoid code
duplication and improve readability - as is the case here. It is quite
common to "hide" error checking logic behind a macro because it is the
error-checking logic that obfuscates the code.
David
-----
-Harsha
On Monday 29 August 2016 02:00 PM, Dmitry Samersoff wrote:
Harsha,
I'm for macro.
You can define a macro right before a place where you uses it and undef
it when you don't need it anymore.
-Dmitry
On 2016-08-25 11:19, Harsha Wardhana B wrote:
Hello All,
Please find below the revised webrev below.
http://cr.openjdk.java.net/~hb/8161448/webrev.01/
Regards
Harsha
On Wednesday 24 August 2016 11:48 AM, Harsha Wardhana B wrote:
Hi David,
On Tuesday 23 August 2016 12:41 PM, David Holmes wrote:
Hi Harsha,
On 22/08/2016 6:30 PM, Harsha Wardhana B wrote:
Hello All,
Please review the below parfait fixes for DiagnosticCommandImpl.c
Issue : https://bugs.openjdk.java.net/browse/JDK-8161448
webrev : http://cr.openjdk.java.net/~hb/8161448/webrev.00/
Looks functionally correct, but I wouldn't complain if you wanted to
use a macro to reduce the duplication and verbosity. Even the:
109 if (obj == NULL) {
110 free(dcmd_arg_info_array);
111 return NULL;
can be replaced by an exception-check as that is the only time
JNU_NewObjectByName can return NULL.
I am not sure if using macro here is good practice since it will be
specific to the function and it will encapsulate the local variable
within it. Also, it will reduce code readability. Hence I would like
to leave it as is.
I also noticed an issue that was flagged in some other JNI code
lately:
213 if (obj == NULL) {
214 free(dcmd_info_array);
215 JNU_ThrowOutOfMemoryError(env, 0);
216 return NULL;
If obj == NULL then there is already a pending exception and we
should not be throwing OOME.
Sure. This needs to be fixed.
Thanks,
David
Regards
Harsha
Harsha
