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

[Dmitry Samersoff]

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.