[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR 8144938: Handle properly coordinate overflow in Marlin Renderer
From:       Laurent_Bourgès <bourges.laurent () gmail ! com>
Date:       2016-03-23 20:41:44
Message-ID: CAKjRUT4nLkyUpgmGi0KLNVkxk-j-4adm82R_BvyyC3RSaEUbbw () mail ! gmail ! com
[Download RAW message or body]

Phil & Jim,

Thanks for the reviews !

I just pushed this patch after a complete synch (jigsaw): build & tested
with MapBench.

Laurent


2016-03-22 20:51 GMT+01:00 Phil Race <philip.race@oracle.com>:

> +1 .. looks like a nice simplification.
>
> Sorry it took me a couple of days to get back to this.
>
> -phil.
>
>
> On 03/18/2016 03:47 PM, Jim Graham wrote:
>
>> The edit looks fine to me...  Phil?
>>
>>         ...jim
>>
>> On 3/18/16 9:52 AM, Laurent Bourgès wrote:
>>
>>> Jim,
>>>
>>> Here is a final webrev (for archive):
>>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.5/
>>>
>>> Changes:
>>> - remove loops in CrashNaNTest
>>>
>>> Hope it is ready to go !
>>>
>>> I need another reviewer, please.
>>>
>>>
>>> 2016-03-18 1:44 GMT+01:00 Jim Graham <james.graham@oracle.com
>>> <mailto:james.graham@oracle.com>>:
>>>
>>>     Hi Laurent,
>>>
>>>     That looks fine.  If the subPathStarted changes are the only changes
>>>     then it's good to go.  If there was some other change that I missed,
>>>     let me know so I can review it as well.
>>>
>>>
>>> No this webrev had only changes in MRE.pathToLoop() you reviewed.
>>>
>>>
>>>     One curiosity - in the test case, why are there 3 for loops that
>>>     perform exactly one pass?  (lines 134, 221, 282)  I'm guessing that
>>>     maybe in a former life this code may have been part of a performance
>>>     suite or something?  It's not a bug, just an odd bit of code.  You
>>>     can delete them if you want, but I don't need to see a review of
>>> that...
>>>
>>>
>>> Fixed, now.
>>>
>>> Cheers,
>>> Laurent
>>>
>>
>


-- 
-- 
Laurent Bourgès

[Attachment #3 (text/html)]

<div dir="ltr"><div><div><div><div><div>Phil &amp; Jim,<br><br></div>Thanks for the \
reviews !<br><br></div>I just pushed this patch after a complete synch (jigsaw): \
build &amp; tested with \
MapBench.<br><br></div><div>Laurent<br></div></div></div><div><div><div><div><br><div><div><div \
class="gmail_extra"><br><div class="gmail_quote">2016-03-22 20:51 GMT+01:00 Phil Race \
<span dir="ltr">&lt;<a href="mailto:philip.race@oracle.com" \
target="_blank">philip.race@oracle.com</a>&gt;</span>:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">+1 .. looks like a nice simplification.<br> <br>
Sorry it took me a couple of days to get back to this.<span class="HOEnZb"><font \
color="#888888"><br> <br>
-phil.</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On 03/18/2016 03:47 PM, Jim Graham wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> The edit looks fine to me...   Phil?<br>
<br>
            ...jim<br>
<br>
On 3/18/16 9:52 AM, Laurent Bourgès wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> Jim,<br>
<br>
Here is a final webrev (for archive):<br>
<a href="http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.5/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.5/</a><br>
 <br>
Changes:<br>
- remove loops in CrashNaNTest<br>
<br>
Hope it is ready to go !<br>
<br>
I need another reviewer, please.<br>
<br>
<br>
2016-03-18 1:44 GMT+01:00 Jim Graham &lt;<a href="mailto:james.graham@oracle.com" \
target="_blank">james.graham@oracle.com</a><br> &lt;mailto:<a \
href="mailto:james.graham@oracle.com" \
target="_blank">james.graham@oracle.com</a>&gt;&gt;:<br> <br>
      Hi Laurent,<br>
<br>
      That looks fine.   If the subPathStarted changes are the only changes<br>
      then it&#39;s good to go.   If there was some other change that I missed,<br>
      let me know so I can review it as well.<br>
<br>
<br>
No this webrev had only changes in MRE.pathToLoop() you reviewed.<br>
<br>
<br>
      One curiosity - in the test case, why are there 3 for loops that<br>
      perform exactly one pass?   (lines 134, 221, 282)   I&#39;m guessing that<br>
      maybe in a former life this code may have been part of a performance<br>
      suite or something?   It&#39;s not a bug, just an odd bit of code.   You<br>
      can delete them if you want, but I don&#39;t need to see a review of \
that...<br> <br>
<br>
Fixed, now.<br>
<br>
Cheers,<br>
Laurent<br>
</blockquote></blockquote>
<br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div \
class="gmail_signature">-- <br>Laurent Bourgès</div> \
</div></div></div></div></div></div></div></div>



[prev in list] [next in list] [prev in thread] [next in thread]