Thanks both for the reviews :) Jc On Wed, Oct 10, 2018 at 10:21 AM Alex Menkov <[email protected]> wrote:
> +1 > > --alex > > On 10/08/2018 15:31, [email protected] wrote: > > Hi Jc, > > > > Thank you for the update! > > It looks good to me. > > > > Thanks, > > Serguei > > > > > > On 10/8/18 14:53, JC Beyler wrote: > >> Hi Serguei, > >> > >> Normally, I try not to fix issues that already exist before the > >> change. For the space after the NULL for example, that would get fixed > >> by https://bugs.openjdk.java.net/browse/JDK-8211335. In this case, > >> these are the only cases where this happens with a NULL so I fixed them. > >> > >> For the space missing before jvmti->X, I fixed my script to handle > >> this correctly and add them automatically. > >> > >> Here is the new webrev: > >> > >> Webrev: http://cr.openjdk.java.net/~jcbeyler/8211801/webrev.01 > >> <http://cr.openjdk.java.net/%7Ejcbeyler/8211801/webrev.01> > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8211801 > >> > >> Thanks for the review! > >> Jc > >> > >> On Mon, Oct 8, 2018 at 12:46 AM [email protected] > >> <mailto:[email protected]> <[email protected] > >> <mailto:[email protected]>> wrote: > >> > >> Hi Jc, > >> > >> > >> > http://cr.openjdk.java.net/%7Ejcbeyler/8211801/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP06/ap06t001/ap06t001.cpp.udiff.html > >> > >> if (!NSK_JNI_VERIFY(jni, (fid = > >> - NSK_CPP_STUB4(GetStaticFieldID, jni, > >> - debugeeClass, > >> - "thread", > >> - THREAD_CLS_SIGNATURE)) != NULL )) { > >> + jni->GetStaticFieldID(debugeeClass, "thread", > >> THREAD_CLS_SIGNATURE)) != NULL )) { > >> nsk_jvmti_setFailStatus(); > >> break; > >> } > >> > >> if (!NSK_JNI_VERIFY(jni, (localRefThread = > >> - NSK_CPP_STUB3(GetStaticObjectField, jni, > >> - debugeeClass, > >> - fid )) != NULL )) { > >> + jni->GetStaticObjectField(debugeeClass, fid)) != NULL )) { > >> NSK_COMPLAIN0("GetStaticObjectField returned NULL for > 'thread' field value\n\n"); > >> nsk_jvmti_setFailStatus(); > >> break; > >> } > >> > >> Could you, please, remove extra space after NULL. > >> > >> > >> In many-many files (especially, in the foleder: > scenarios/capability): > >> > >> + if > >> > > (!NSK_JVMTI_VERIFY_CODE(JVMTI_ERROR_MUST_POSSESS_CAPABILITY,jvmti->SuspendThread(thread))) > >> ... + if > >> > > (!NSK_JVMTI_VERIFY_CODE(JVMTI_ERROR_MUST_POSSESS_CAPABILITY,jvmti->ResumeThread(thread))) > >> ... + if > >> > > (!NSK_JVMTI_VERIFY_CODE(JVMTI_ERROR_MUST_POSSESS_CAPABILITY,jvmti->InterruptThread(thread))) > >> > >> > >> There are a lot of cases like above where a space before jvmti-> > >> is missed. > >> > >> > >> Otherwise, looks good. > >> > >> > >> Thanks! > >> Serguei > >> > >> > >> On 10/5/18 16:50, JC Beyler wrote: > >>> Hi all, > >>> > >>> I continued the NSK_CPP_STUB removal with this next webrev. > >>> > >>> My apologies, this one is big for 50 files; I can further cut it > >>> up if we prefer. It is straight-forward though, since it is just > >>> doing the same NSK_CPP_STUB removal. > >>> > >>> Without further ado: > >>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8211801/webrev.00/ > >>> <http://cr.openjdk.java.net/%7Ejcbeyler/8211801/webrev.00/> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8211801 > >>> > >>> This does another 50 file batch. I did the same for > >>> https://bugs.openjdk.java.net/browse/JDK-8211782 so the scripts > >>> can be found there (and added a comment in this bug to link to > that). > >>> > >>> I've tested the modified changes on my machine, all modified > >>> tests pass. > >>> > >>> Let me know what you think, > >>> Jc > >> > >> > >> > >> -- > >> > >> Thanks, > >> Jc > > > -- Thanks, Jc
