RFR (L) 8212770: Remove spaces before/after () for vmTestbase/jvmti/[s-u]

JC Beyler jcbeyler at google.com
Thu Oct 25 02:54:05 UTC 2018


Hi Chris,

I inlined my answers:

On Wed, Oct 24, 2018 at 2:58 PM Chris Plummer <chris.plummer at oracle.com>
wrote:

> 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) {
>
>
I can't find it so I created:
https://bugs.openjdk.java.net/browse/JDK-8212959



> 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:
https://bugs.openjdk.java.net/browse/JDK-8212960

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
>
> On Mon, Oct 22, 2018 at 1:37 PM Alex Menkov <alexey.menkov at oracle.com>
> wrote:
>
>> LGTM.
>>
>> --alex
>>
>> On 10/22/2018 11:29, JC Beyler wrote:
>> > Hi all,
>> >
>> > Here is the next webrev (second out of 3) to remove the spaces
>> > after/before () from vmTestbase. It is straightforward and I generated
>> > the webrev with white-space changes showing up of course:
>> >
>> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/
>> > <http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/>
>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8212770
>> >
>> > Could I please get some reviews?
>> >
>> > Thanks,
>> > Jc
>>
>
>
> --
>
> Thanks,
> Jc
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181024/5e713b73/attachment.html>


More information about the serviceability-dev mailing list