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.
