Review request: JDK-8170030 Code in Marlin-based rasterizers may have an off-by-1 bug
james.graham at oracle.com
Fri Dec 9 18:50:36 UTC 2016
Normally we'd isolate fixes for different bugs even if they are just preparatory formatting changes on comments.
Some comments on the formatting changes:
line 35 - you should also upper-case the E
line 148 - I don't understand why this "float" was a formatting danger, but the one on line 328 isn't? Not a big issue,
but curious since I think the edges array does contain floats in it, doesn't it?
line 328 - (ints) => [ints] to match? (Also would match line 148)
Comments on the bug fix:
line 1401 - why does maxY want to be bounds+1?
- later this is used to calculate buckets_maxY
- do we want an extra Y bucket for some reason?
- and why only in case of spmaxY was clipped?
- a comment about that would help
Other than that one odd case of using "+1" there, that whole block of code is just:
spmaxY = min(edgeMaxY, _boundsMaxY)
which is much easier to read.
Perhaps we need an extra bucket only when there were clipped edges below the clip?
line 1456 - I can understand why we lose the +1, but not the min() with pmaxY?
NoAA comments are identical, though line numbers may be different...
On 12/9/16 8:46 AM, Laurent Bourgès wrote:
> Please review this simple fix for MarlinFX:
> JBS: https://bugs.openjdk.java.net/browse/JDK-8170030
> webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170030.0/
> PS: I will provide asap a Marlin2D patch incorporating changes made in
> MarlinFX (including this one)
More information about the openjfx-dev