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

[David Holmes]

Hi Harsha,

Sorry these style issues are proving to be so painful, normally there 
would be more direct guidance from an existing team member.

On 30/08/2016 4:51 PM, Harsha Wardhana B wrote:
> Hello,
>
> Please find below webrev addressing David's and Dmitry's comments.
>
> http://cr.openjdk.java.net/~hb/8161448/webrev.02/

The idiomatic way we write such macros is as follows:

#define EXCEPTION_CHECK_AND_FREE(x) \
   do { \
     if ((*env)->ExceptionCheck(env)) { \
        free(x); \
        return NULL; \
     } \
   } while (false)

---

  109     if (obj == NULL) {
  110         EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
  111         }

Checking both NULL and for exceptions is redundant as previously pointed 
out. Either leave this code as it was:

91     if (obj == NULL) {
92       free(dcmd_arg_info_array);
93       return NULL;
94     }

or else just replace it with:

109    EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);

The same for the block here:

  201       if (obj == NULL){
  202         EXCEPTION_CHECK_AND_FREE(dcmd_info_array);
  203       }


Thanks,
David
-----

> -Harsha
>
> On Tuesday 30 August 2016 09:46 AM, David Holmes wrote:
>> 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
>>>>>>
>>>>>
>>>
>