RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
David Holmes
david.holmes at oracle.com
Mon Aug 29 06:56:01 UTC 2016
Hi Harsha,
On 29/08/2016 8:12 PM, Harsha Wardhana B wrote:
> 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
No hard and fast rules I'm afraid. The proposed Java style guide can
also apply to native code:
http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html
and we have the hotspot coding guidelines:
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
A line length of 80 is only a guide (and many would says it is a guide
from the 1970's!).
There is also the general "rule" that unless something is terribly
broken then consistency with the existing style is important - we don't
want to have a dozen different styles at play in one file/class/method.
The things you changed did not really need to be changed, but if you
insist on changing them then I would strongly argue for a different
style with regard to the indent ie instead of
"JMX interface to diagnostic framework notifications "
"is not supported by this VM");
it should be
"JMX interface to diagnostic framework notifications "
"is not supported by this VM");
And:
70 dcmdArgInfoCls = (*env)->FindClass(
71 env,
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
should be:
70 dcmdArgInfoCls = (*env)->FindClass(env,
71
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
or if that is too jarring due to the length of line 71 then it could be:
70 dcmdArgInfoCls =
71 (*env)->FindClass(env,
72
"com/sun/management/internal/DiagnosticCommandArgumentInfo");
But unless something is really, really bad I would resist the urge to do
these kinds of "clean ups" as they just obscure the real changes and
make the review harder (and longer).
Thanks,
David
> 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
>>>
>
More information about the serviceability-dev
mailing list