RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c

[David Holmes]

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