I see. I asumme that such test will be added when we introduce such modules and update IsModifiableModule method, however I'd recommend to file an RFE so we won't forget about it later.
Cheers, -- Igor > On Jun 13, 2017, at 3:12 PM, serguei.spit...@oracle.com wrote: > > Hi Igor, > > Thank you a lot for review! > There are no non-modifiable modules in jdk 9. > So that it is impossible to test that IsModifiableModule returns false. > Thank you for the suggestion, I'll replace the Exception with > java/lang/AssertionError. > > Thanks, > Serguei > > > On 6/13/17 15:07, Igor Ignatyev wrote: >> HI Serguei, >> >> generally it looks good, but you still need at least one test to check that >> IsModifiableModule returns false than it must. >> >> minor nit: I'd prefer to have java/lang/AssertionError instead of Exception >> as EXC_CNAME in libIsModifiableModuleTest.c as it clearly shows that some of >> test checks failed. >> >> Thanks, >> -- Igor >> >>> On Jun 13, 2017, at 1:56 PM, serguei.spit...@oracle.com >>> <mailto:serguei.spit...@oracle.com> wrote: >>> >>> PING.. >>> >>> This is for jdk 9. >>> I have one review from Alan and need at least one more. >>> Added hotspot-test and hotspot_dev_confidential mailing lists to the "To:" >>> list. >>> >>> Thanks, >>> Serguei >>> >>> >>> On 6/10/17 04:08, serguei.spit...@oracle.com >>> <mailto:serguei.spit...@oracle.com> wrote: >>>> >>>> >>>> On 6/10/17 00:41, Alan Bateman wrote: >>>>> >>>>> >>>>> On 09/06/2017 19:32, serguei.spit...@oracle.com >>>>> <mailto:serguei.spit...@oracle.com> wrote: >>>>>> >>>>>> Ok, I removed the get_class_loader function, replaced the loader with a >>>>>> class and left alone the "not_a_module" parameter. >>>>>> This is the simplest way to provide a non-module jobject. >>>>>> Otherwise, the JNIEnv parameter has to be passed to the >>>>>> check_is_modifiable_error_codes . >>>>>> Also, I've done a small refactoring by inlining the >>>>>> test_is_modifiable_module body into the *_check function. >>>>>> >>>>>> New webrev: >>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8178054-jvmti-ismodif.4/ >>>>>> >>>>>> <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2017/hotspot/8178054-jvmti-ismodif.4/> >>>>>> >>>>> I think it looks good now. >>>> >>>> Thank you a lot for review and good suggestions! >>>> Serguei >>>> >>>>> >>>>> -Alan. >>>> >>> >> >
