[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
From:       Harsha Wardhana B <harsha.wardhana.b () oracle ! com>
Date:       2016-08-30 6:52:34
Message-ID: 376c70be-7856-ec54-d8b6-0e7127c39c80 () oracle ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]