On Wed, 15 Dec 2021 23:49:24 GMT, Phil Race <p...@openjdk.org> wrote:
>> Jeremy has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control >> points in bounding box >> >> Addressing code review feedback: "Use BigDecomal.setScale(40) to ascertain >> precision." >> >> The geom.* unit tests passed before & after this change. >> >> https://github.com/openjdk/jdk/pull/6227#discussion_r752908494 > > There seems to be a number of things to discuss aside from the maths > > - You are changing the signature of public API > src/java.desktop/share/classes/java/awt/geom/Path2D.java > public final synchronized Rectangle2D getBounds2D() => public Rectangle2D > getBounds2D() { > > So no longer final, and no longer synchronized. > This means a CSR is required and we need to think about it .. the intention > was that the subclass not over-ride. > And why remove synchronized ? I am fairly sure it was there to make sure no > one was mutating the Path whilst > bounds are being calculated. > And you are using getPathIterator(AffineTransform) and the docs for that say > it isn't thread safe. > So I think this implementation needs to be thought about very carefully. > > - You are adding a new public static method > > public static Rectangle2D getBounds2D(final PathIterator pi); > > Is this really necessary ? It is just for the benefit of the public API > caller so can be package private. > > - For me it doesn't build because of a doclint error > > src/java.desktop/share/classes/java/awt/geom/Path2D.java:2102: warning: no > @param for pi > public static Rectangle2D getBounds2D(final PathIterator pi) { > ^ > error: warnings found and -Werror specified > > Fixing that I find that the updated UnitTest.java fail with a build with > these changes :- > > % java UnitTest > Exception in thread "main" java.lang.RuntimeException: the shape must not > intersect anything above its bounds > at UnitTest.testGetBounds2D(UnitTest.java:1396) > at UnitTest.testBounds(UnitTest.java:1302) > at UnitTest.test(UnitTest.java:1175) > at UnitTest.main(UnitTest.java:1435) > > Then we have some even more difficult questions. > The control points may not be within the bounds of the curve which is of > course the premise of the bug report. > The specification (javadoc) isn't very forthcoming on whether it is allowed > to not include them .. ie is it expected they should be ? > And it appears we have JCK tests which expect they ARE included. So as it is > this change would not pass the existing TCK/JCK. > Which means we'd have to be 100% sure about this change in behaviour and be > able to justify it. > FWIW unless the spec explicitly says they are, in some place I've overlooked, > then I'd say its a quality-of-implementation thing as to how tight the bounds > are, and the new behaviour is allowed by spec. > Applications however might still find this a behaviourally incompatible > change .. and could potentially see undesirable consequences - > even as other applications see better consequences. > So perhaps new API that defines tighter bounds might be the way to address > this .. but I am far from being sure about that. @prrace could you say on CSR or not ? ------------- PR: https://git.openjdk.java.net/jdk/pull/6227
