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
