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

[Harsha Wardhana B]

Hi David,

The reason I fixed the indent was to maintain 80 character line width. I 
am not too familiar with coding conventions being followed w.r.t 
indentations. Could you point me to coding conventions for native code 
if we are following anything of that sort

Thanks
Harsha

On Friday 26 August 2016 04:01 AM, David Holmes wrote:
> On 25/08/2016 6:19 PM, Harsha Wardhana B wrote:
>> Hello All,
>>
>> Please find below the revised webrev below.
>>
>> http://cr.openjdk.java.net/~hb/8161448/webrev.01/
>
> Functional changes seem okay. Exception management seems consistent.
>
> You have made numerous other incidental changes which not only make it 
> harder to examine this incrementally, but in most cases are wrong:
>
>  36                         "JMX interface to diagnostic framework 
> notifications "
>   37                 "is not supported by this VM");
>
> The indentation is wrong after splitting the line. The strings should 
> line up.
>
>   62   /* According to ISO C it is perfectly legal for malloc to 
> return zero if
>   63    * called with a zero argument */
>
> A multi-line comment should end with the */ on its own line.
>
>   70   dcmdArgInfoCls = (*env)->FindClass(
>   71           env, 
> "com/sun/management/internal/DiagnosticCommandArgumentInfo");
>
> This indent is wrong, the original was correct.
>
>  183       args = getDiagnosticCommandArgumentInfoArray(
>  184               env, (*env)->GetObjectArrayElement(env,commands,i),
>  185               dcmd_info_array[i].num_arguments);
>
> Indent is wrong, original was correct.
>
> 207       obj = JNU_NewObjectByName(
>  208               env, 
> "com/sun/management/internal/DiagnosticCommandInfo",
>  209 "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
>  210 "Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
>  211               "ZLjava/util/List;)V",
>  212               jname, jdesc, jimpact,
>  213               dcmd_info_array[i].permission_class==NULL?NULL:
>  214 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
>  215               dcmd_info_array[i].permission_name==NULL?NULL:
>  216 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_name),
>  217 dcmd_info_array[i].permission_action==NULL?NULL:
>  218 (*env)->NewStringUTF(env,dcmd_info_array[i].permission_action),
>  219               dcmd_info_array[i].enabled, args);
>
> Again indent is wrong (yes those really long strings are a pain but 
> that's life) and the original was more correct ie NewObjectByName(env 
> on first line, then other args line up underneath.
>
> However if you are going to fix layout in this bit of code then please 
> add spaces around the operators ie:
>
> cmd_info_array[i].permission_class == NULL ? NULL :
>
> (*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
>
> Thanks,
> David
>
>> 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
>>