Dmitry,
The scope of the issue was to fix parfait warnings though I have gone to
some extent to refactor the file.
Agree that macro can be used, but we have to stop here and raise a new
issue to carry-on with the refactoring process.
Thanks
Harsha
On Tuesday 06 September 2016 12:03 PM, Dmitry Samersoff wrote:
Harsha,
EXCEPTION_CHECK_AND_FREE macro could be used at ll.76 after FindClass
-Dmitry
On 2016-09-06 08:47, Harsha Wardhana B wrote:
Hi David,
Please find revised webrev.
http://cr.openjdk.java.net/~hb/8161448/webrev.03/
-Harsha
On Thursday 01 September 2016 03:31 PM, David Holmes wrote:
On 1/09/2016 6:05 PM, Harsha Wardhana B wrote:
Hi David,
On Thursday 01 September 2016 01:14 PM, David Holmes wrote:
Hi Harsha,
Sorry these style issues are proving to be so painful, normally there
would be more direct guidance from an existing team member.
On 30/08/2016 4:51 PM, Harsha Wardhana B wrote:
Hello,
Please find below webrev addressing David's and Dmitry's comments.
http://cr.openjdk.java.net/~hb/8161448/webrev.02/
The idiomatic way we write such macros is as follows:
#define EXCEPTION_CHECK_AND_FREE(x) \
do { \
if ((*env)->ExceptionCheck(env)) { \
free(x); \
return NULL; \
} \
} while (false)
I am aware of this style but I wasn't aware what style should be used
(BTW last line should be 'while(0)'). I didn't want to do anything fancy
and end up sending one more webrev and hence I followed the simplest way
to write a multi-line macro.
Now if there isn't anything wrong, I think we should leave it as is
until coding guidelines are put in place. I am facing same problem in
another issue where I have sent multiple webrev fixing only nits. It is
turning out to be very painful.
I understand and it is unfortunate that there has not been more
definitive guidance here. But it is better to adopt established style
and get used to using it. If you form is left in then you need spaces
before trailing \ - another style nit, sorry.
---
109 if (obj == NULL) {
110 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
111 }
Checking both NULL and for exceptions is redundant as previously
pointed out. Either leave this code as it was:
Yes. it may be redundant but it is harmless. Does this really need to be
changed?
Yes it breeds confusion to see multiple checks that are unnecessary
(and we're trying to address this with a lot of the JNI using code in
the VM at the moment). In this case it is even worse because without
knowing the exception check must be true and so "return NULL" will
always execute, this code has the appearance of being able to continue
if obj == NULL!
Sorry. And thanks.
David
-----
91 if (obj == NULL) {
92 free(dcmd_arg_info_array);
93 return NULL;
94 }
or else just replace it with:
109 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
The same for the block here:
201 if (obj == NULL){
202 EXCEPTION_CHECK_AND_FREE(dcmd_info_array);
203 }
Thanks,
David
-----
-Harsha
On Tuesday 30 August 2016 09:46 AM, David Holmes wrote:
On 30/08/2016 2:06 PM, Harsha Wardhana B wrote:
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
You claimed macros obfuscate so the same word seemed appropriate. I
don't necessarily equate readability with obfuscation.
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.
I was envisaging something like:
jstring jname, jdesc,jtype,jdefStr;
jname = (*env)->NewStringUTF(env,dcmd_arg_info_array[i].name);
EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
jdesc =
(*env)->NewStringUTF(env,dcmd_arg_info_array[i].description);
EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
jtype = (*env)->NewStringUTF(env,dcmd_arg_info_array[i].type);
EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
jdefStr = (*env)->NewStringUTF(env,
dcmd_arg_info_array[i].default_string);
EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
Of course if this were C++ code instead of C there would be better
techniques for dealing with the free'ing.
Cheers,
David
-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
Regards
Harsha
