RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Harsha Wardhana B
harsha.wardhana.b at oracle.com
Thu Sep 1 08:05:34 UTC 2016
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:
>
> 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
More information about the serviceability-dev
mailing list