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

[Harsha Wardhana B]

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.

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