Looks good! Thanks, /Staffan
> On 9 dec 2014, at 15:10, serguei.spit...@oracle.com wrote: > > Dmitry, > > It looks good. > > Than you for the update! > Serguei > > On 12/9/14 5:51 AM, Dmitry Samersoff wrote: >> 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. >> >
