Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Thanks for the review, Dmitry.
Harsha
On Tuesday 06 September 2016 12:27 PM, Dmitry Samersoff wrote:
Harsha,
OK. The fix looks good for me.
-Dmitry
On 2016-09-06 09:49, Harsha Wardhana B wrote:
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:
109EXCEPTION_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++
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Harsha,
OK. The fix looks good for me.
-Dmitry
On 2016-09-06 09:49, Harsha Wardhana B wrote:
> 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:
>>
>> 109EXCEPTION_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);
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Thanks for the review David.
On Tuesday 06 September 2016 11:35 AM, David Holmes wrote:
Hi Harsha,
On 6/09/2016 3:47 PM, Harsha Wardhana B wrote:
Hi David,
Please find revised webrev.
http://cr.openjdk.java.net/~hb/8161448/webrev.03/
You lost the earlier fix to not throw the exception here:
202 if (obj == NULL) {
203 free(dcmd_info_array);
204 JNU_ThrowOutOfMemoryError(env, 0);
205 return NULL;
206 }
but otherwise this all looks fine. No need to see an updated webrev
with line 204 deleted.
Thanks for your patience and perseverance on this one.
David
-
-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:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Hi Harsha,
On 6/09/2016 3:47 PM, Harsha Wardhana B wrote:
Hi David,
Please find revised webrev.
http://cr.openjdk.java.net/~hb/8161448/webrev.03/
You lost the earlier fix to not throw the exception here:
202 if (obj == NULL) {
203 free(dcmd_info_array);
204 JNU_ThrowOutOfMemoryError(env, 0);
205 return NULL;
206 }
but otherwise this all looks fine. No need to see an updated webrev with
line 204 deleted.
Thanks for your patience and perseverance on this one.
David
-
-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:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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:
109EXCEPTION_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.
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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.
---
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?
91 if (obj == NULL) {
92 free(dcmd_arg_info_array);
93 return NULL;
94 }
or else just replace it with:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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)
---
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:
91 if (obj == NULL) {
92 free(dcmd_arg_info_array);
93 return NULL;
94 }
or else just replace it with:
109EXCEPTION_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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Hello,
Please find below webrev addressing David's and Dmitry's comments.
http://cr.openjdk.java.net/~hb/8161448/webrev.02/
-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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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.
-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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Hi Harsha,
On 29/08/2016 8:12 PM, Harsha Wardhana B wrote:
Hi David,
The reason I fixed the indent was to maintain 80 character line width. I
am not too familiar with coding conventions being followed w.r.t
indentations. Could you point me to coding conventions for native code
if we are following anything of that sort
No hard and fast rules I'm afraid. The proposed Java style guide can
also apply to native code:
http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html
and we have the hotspot coding guidelines:
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
A line length of 80 is only a guide (and many would says it is a guide
from the 1970's!).
There is also the general "rule" that unless something is terribly
broken then consistency with the existing style is important - we don't
want to have a dozen different styles at play in one file/class/method.
The things you changed did not really need to be changed, but if you
insist on changing them then I would strongly argue for a different
style with regard to the indent ie instead of
"JMX interface to diagnostic framework notifications "
"is not supported by this VM");
it should be
"JMX interface to diagnostic framework notifications "
"is not supported by this VM");
And:
70 dcmdArgInfoCls = (*env)->FindClass(
71 env,
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
should be:
70 dcmdArgInfoCls = (*env)->FindClass(env,
71
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
or if that is too jarring due to the length of line 71 then it could be:
70 dcmdArgInfoCls =
71 (*env)->FindClass(env,
72
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
But unless something is really, really bad I would resist the urge to do
these kinds of "clean ups" as they just obscure the real changes and
make the review harder (and longer).
Thanks,
David
Thanks
Harsha
On Friday 26 August 2016 04:01 AM, David Holmes wrote:
On 25/08/2016 6:19 PM, Harsha Wardhana B wrote:
Hello All,
Please find below the revised webrev below.
http://cr.openjdk.java.net/~hb/8161448/webrev.01/
Functional changes seem okay. Exception management seems consistent.
You have made numerous other incidental changes which not only make it
harder to examine this incrementally, but in most cases are wrong:
36 "JMX interface to diagnostic framework
notifications "
37 "is not supported by this VM");
The indentation is wrong after splitting the line. The strings should
line up.
62 /* According to ISO C it is perfectly legal for malloc to
return zero if
63* called with a zero argument */
A multi-line comment should end with the */ on its own line.
70 dcmdArgInfoCls = (*env)->FindClass(
71 env,
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
This indent is wrong, the original was correct.
183 args = getDiagnosticCommandArgumentInfoArray(
184 env, (*env)->GetObjectArrayElement(env,commands,i),
185 dcmd_info_array[i].num_arguments);
Indent is wrong, original was correct.
207 obj = JNU_NewObjectByName(
208 env,
"com/sun/management/internal/DiagnosticCommandInfo",
209 "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
210 "Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
211 "ZLjava/util/List;)V",
212 jname, jdesc, jimpact,
213 dcmd_info_array[i].permission_class==NULL?NULL:
214 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
215 dcmd_info_array[i].permission_name==NULL?NULL:
216 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_name),
217 dcmd_info_array[i].permission_action==NULL?NULL:
218 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_action),
219 dcmd_info_array[i].enabled, args);
Again indent is wrong (yes those really long strings are a pain but
that's life) and the original was more correct ie NewObjectByName(env
on first line, then other args line up underneath.
However if you are going to fix layout in this bit of code then please
add spaces around the operators ie:
cmd_info_array[i].permission_class == NULL ? NULL :
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
Thanks,
David
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Hi David,
The reason I fixed the indent was to maintain 80 character line width. I
am not too familiar with coding conventions being followed w.r.t
indentations. Could you point me to coding conventions for native code
if we are following anything of that sort
Thanks
Harsha
On Friday 26 August 2016 04:01 AM, David Holmes wrote:
On 25/08/2016 6:19 PM, Harsha Wardhana B wrote:
Hello All,
Please find below the revised webrev below.
http://cr.openjdk.java.net/~hb/8161448/webrev.01/
Functional changes seem okay. Exception management seems consistent.
You have made numerous other incidental changes which not only make it
harder to examine this incrementally, but in most cases are wrong:
36 "JMX interface to diagnostic framework
notifications "
37 "is not supported by this VM");
The indentation is wrong after splitting the line. The strings should
line up.
62 /* According to ISO C it is perfectly legal for malloc to
return zero if
63* called with a zero argument */
A multi-line comment should end with the */ on its own line.
70 dcmdArgInfoCls = (*env)->FindClass(
71 env,
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
This indent is wrong, the original was correct.
183 args = getDiagnosticCommandArgumentInfoArray(
184 env, (*env)->GetObjectArrayElement(env,commands,i),
185 dcmd_info_array[i].num_arguments);
Indent is wrong, original was correct.
207 obj = JNU_NewObjectByName(
208 env,
"com/sun/management/internal/DiagnosticCommandInfo",
209 "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
210 "Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
211 "ZLjava/util/List;)V",
212 jname, jdesc, jimpact,
213 dcmd_info_array[i].permission_class==NULL?NULL:
214 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
215 dcmd_info_array[i].permission_name==NULL?NULL:
216 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_name),
217 dcmd_info_array[i].permission_action==NULL?NULL:
218 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_action),
219 dcmd_info_array[i].enabled, args);
Again indent is wrong (yes those really long strings are a pain but
that's life) and the original was more correct ie NewObjectByName(env
on first line, then other args line up underneath.
However if you are going to fix layout in this bit of code then please
add spaces around the operators ie:
cmd_info_array[i].permission_class == NULL ? NULL :
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
Thanks,
David
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
On 25/08/2016 6:19 PM, Harsha Wardhana B wrote:
Hello All,
Please find below the revised webrev below.
http://cr.openjdk.java.net/~hb/8161448/webrev.01/
Functional changes seem okay. Exception management seems consistent.
You have made numerous other incidental changes which not only make it
harder to examine this incrementally, but in most cases are wrong:
36 "JMX interface to diagnostic framework
notifications "
37 "is not supported by this VM");
The indentation is wrong after splitting the line. The strings should
line up.
62 /* According to ISO C it is perfectly legal for malloc to return
zero if
63* called with a zero argument */
A multi-line comment should end with the */ on its own line.
70 dcmdArgInfoCls = (*env)->FindClass(
71 env,
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
This indent is wrong, the original was correct.
183 args = getDiagnosticCommandArgumentInfoArray(
184 env, (*env)->GetObjectArrayElement(env,commands,i),
185 dcmd_info_array[i].num_arguments);
Indent is wrong, original was correct.
207 obj = JNU_NewObjectByName(
208 env,
"com/sun/management/internal/DiagnosticCommandInfo",
209
"(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
210 "Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
211 "ZLjava/util/List;)V",
212 jname, jdesc, jimpact,
213 dcmd_info_array[i].permission_class==NULL?NULL:
214
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
215 dcmd_info_array[i].permission_name==NULL?NULL:
216
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_name),
217 dcmd_info_array[i].permission_action==NULL?NULL:
218
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_action),
219 dcmd_info_array[i].enabled, args);
Again indent is wrong (yes those really long strings are a pain but
that's life) and the original was more correct ie NewObjectByName(env on
first line, then other args line up underneath.
However if you are going to fix layout in this bit of code then please
add spaces around the operators ie:
cmd_info_array[i].permission_class == NULL ? NULL :
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
Thanks,
David
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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
Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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 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.
Thanks,
David
Regards
Harsha
RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
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/ Regards Harsha
