|
Hi JC,
webrev.00 is fine since you have bugs filed for the rest.
thanks,
Chris
On 10/24/18 7:54 PM, JC Beyler wrote:
Hi Chris,
I inlined my answers:
Hi
JC,
Overall looks pretty good:
I see some cases of changes on lines where there is
an assignment in a conditional. Will these conflict
with your other webrev?
if (!NSK_JNI_VERIFY(jni, (root =
-
jni->GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL )) {
+
jni->GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL)) {
Yes they might, I was going to go with the first that
goes through and send an incremental for the other one.
To be honest, I kind of was expecting perhaps some more
conversation about extracting the assignments that I'd
have time to fix the conflicts without anyone really
noticing :)
I noticed a number of cases of checking if a boolean
result is true or false. I brought this up once
before. I forgot if you filed a separate CR for it:
- if (nsk_jvmti_parseOptions(options) ==
NSK_FALSE ) {
+ if (nsk_jvmti_parseOptions(options) ==
NSK_FALSE) {
The following is missing a space. This piece of code
is probably replicated at least a dozen times.
- if ( rc!= JNI_OK ) {
+ if (rc!= JNI_OK) {
And a missing space here also. Only saw it in one
place.
- if ( (strcmp(name, CLASS_NAME) ==0 ) &&
(redefineNumber == 0) ) {
+ if ((strcmp(name, CLASS_NAME) ==0) &&
(redefineNumber == 0)) {
There's a double space here:
- if ( nsk_jvmti_redefineClass(jvmti_env,
klass, fileName) == NSK_TRUE ) {
+ if (nsk_jvmti_redefineClass(jvmti_env,
klass, fileName) == NSK_TRUE) {
For all of these, I created:
Let me know if these answer your issues and if
therefore I can push webrev.00 in your opinion. Then
I'll work on the potential conflicts for the other
webrev,
Jc
thanks,
Chris
On 10/24/18 1:00 PM, JC Beyler wrote:
Hi all,
Anybody else want to review this? We can
close the book on spaces before/after () once
this one goes in :)
Jc
--
--
|
