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

[Harsha Wardhana B]

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

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