Serguei, Fixed. Webrev updated in-place, please press shift-reload.
http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.03/ -Dmitry On 2014-12-08 14:02, serguei.spit...@oracle.com wrote: > Looks good. > > Some minor comments. > > Better to reformat with one variable at a line: > > 113 const char *error_message = NULL, *jrepath = NULL, *libname = NULL; > > 283 jbyte *start, *end; > > > Uninitialized locals: > > 115 #ifdef _WINDOWS > 116 HINSTANCE hsdis_handle; > 117 #else > 118 void* hsdis_handle; > 119 #endif > ... > 201 jlong result; > ... > > 282 jboolean isCopy; > 283 jbyte *start, *end; > 284 jclass disclass; > 285 const char *options; > > > Unaligned parameters: > > 209 result = (*env)->CallLongMethod(env, denv->dis, denv->handle_event, > denv->visitor, > 210 event_string, (jlong) > (uintptr_t)arg); > > > Thanks, > Serguei > > > On 12/7/14 6:17 AM, Dmitry Samersoff wrote: >> Please, review a modified fix: >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.03/ >> >> Windows compiler doesn't allow declaration in the middle of the function >> for c code. >> >> Also I put my few cents to reduce build noise and add a pragma that >> disable windows compiler warnings that have no value in this context. >> >> -Dmitry >> >> >> On 2014-12-03 16:01, Staffan Larsen wrote: >>> Changes look good. What testing have you done? >>> >>> /Staffan >>> >>>> On 3 dec 2014, at 13:06, Dmitry Samersoff <dmitry.samers...@oracle.com> >>>> wrote: >>>> >>>> Serguei, >>>> >>>> Updated webrev >>>> >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.02/ >>>> >>>> -Dmitry >>>> >>>> On 2014-12-03 01:24, serguei.spit...@oracle.com wrote: >>>>> Dmitry, >>>>> >>>>> It is good in general modulo Staffan's comments. >>>>> >>>>> There are some inconsistencies: >>>>> - the ExceptionOccurred(env) is compared to != NULL (which make the >>>>> logic more complex) >>>>> in some places and no such comparison (implicit comparison instead) >>>>> in others >>>>> - two different forms of check/action are used: >>>>> - (*env)->ExceptionOccurred(env) and >>>>> - !(*env)->ExceptionOccurred(env) >>>>> >>>>> I'd suggest to do it the same way and always return the "default" value >>>>> if an exception occurred. >>>>> It will make the logic more flat and clear. >>>>> Otherwise, we have to think what exact value is returned in exception >>>>> occurred cases like at lines 241, 255. >>>>> >>>>> The comment // OOM is used inconsistently too. >>>>> >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> >>>>> On 12/2/14 11:14 AM, Dmitry Samersoff wrote: >>>>>> Please review the small fix. >>>>>> >>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.01/ >>>>>> >>>>>> Added more missed exception checks to sadis.c >>>>>> >>>> >>>> -- >>>> Dmitry Samersoff >>>> Oracle Java development team, Saint Petersburg, Russia >>>> * I would love to change the world, but they won't give me the sources. >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
