Hi JC, thanks for your comments . Second webrev , may I have a second review please ?
http://cr.openjdk.java.net/~mbaesken/webrevs/8220355.1/ One question that came to my mind : do we still have to worry about the old-style C variable declaration order in eventHandlerVMInit after changing the test to if (environment == NULL) { … } or is this resolved now on all platforms ? Best regards, Matthias From: Jean Christophe Beyler <jcbey...@google.com> Sent: Freitag, 15. März 2019 15:53 To: Baesken, Matthias <matthias.baes...@sap.com> Cc: serviceability-dev@openjdk.java.net Subject: Re: RFR: 8220355: Improve assertion texts and exception messages in eventHandlerVMInit Hi Matthias, (Not an official reviewer :-)) http://cr.openjdk.java.net/~mbaesken/webrevs/8220355.0/: - I would invert the test for the environment and do: if (environment == NULL) { abortJVM(jnienv, JPLIS_ERRORMESSAGE_CANNOTSTART ", getting JPLIS environment failed"); } and then just un-indent the rest of the code that was in the original if. http://cr.openjdk.java.net/~mbaesken/webrevs/8220355.0/src/java.instrument/share/native/libinstrument/JPLISAgent.c.udiff.html - Nit: the message could consistently start with upper case or lower case :-) Apart from that, looks good to me :-) Thanks, Jc On Fri, Mar 15, 2019 at 2:19 AM Baesken, Matthias <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>> wrote: Hello, please review the following change . It enhances the error messages in libinstrument in case initialization fails . ( I found the enhanced error messages helpful when analyzing jtreg failures on one of our platforms in the java/lang/instrument/PremainClass/PremainClassTest.java ) Bug/webrev : https://bugs.openjdk.java.net/browse/JDK-8220355 http://cr.openjdk.java.net/~mbaesken/webrevs/8220355.0/ Thanks, Matthias -- Thanks, Jc
