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

[David Holmes]

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