<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Looks good.<br>
<br>
Some minor comments.<br>
<br>
Better to reformat with one variable at a line:<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre><span class="changed"> 113 const char *error_message = NULL, *jrepath = NULL, *libname = NULL;</span></pre>
<pre><span class="changed"> 283 jbyte *start, *end;</span></pre>
<br>
Uninitialized locals:<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre><span class="new"> 115 #ifdef _WINDOWS</span>
<span class="new"> 116 HINSTANCE hsdis_handle;</span>
<span class="new"> 117 #else</span>
<span class="new"> 118 void* hsdis_handle;</span>
<span class="new"> 119 #endif
...
</span><meta http-equiv="content-type" content="text/html; charset=windows-1252"><span class="new"> 201 jlong result;
...
</span></pre>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre> 282 jboolean isCopy;
<span class="changed"> 283 jbyte *start, *end;</span>
<span class="changed"> 284 jclass disclass;</span>
<span class="changed"> 285 const char *options;</span>
</pre>
<br>
Unaligned parameters:<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre><span class="changed"> 209 result = (*env)->CallLongMethod(env, denv->dis, denv->handle_event, denv->visitor,</span>
210 event_string, (jlong) (uintptr_t)arg);</pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 12/7/14 6:17 AM, Dmitry Samersoff wrote:<br>
</div>
<blockquote cite="mid:54846185.3050508@oracle.com" type="cite">
<pre wrap="">Please, review a modified fix:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.03/">http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.03/</a>
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:
</pre>
<blockquote type="cite">
<pre wrap="">Changes look good. What testing have you done?
/Staffan
</pre>
<blockquote type="cite">
<pre wrap="">On 3 dec 2014, at 13:06, Dmitry Samersoff <a class="moz-txt-link-rfc2396E" href="mailto:dmitry.samersoff@oracle.com"><dmitry.samersoff@oracle.com></a> wrote:
Serguei,
Updated webrev
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.02/">http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.02/</a>
-Dmitry
On 2014-12-03 01:24, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">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:
</pre>
<blockquote type="cite">
<pre wrap="">Please review the small fix.
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.01/">http://cr.openjdk.java.net/~dsamersoff/JDK-8028773/webrev.01/</a>
Added more missed exception checks to sadis.c
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>