TransformUtils.java - can't this class be deleted now since all it
exists to do is to forward to TransformHelper?
Alternatively, why create TransformHelper in the first place when it is
90% just a copy of what TransformUtils used to be (ignoring the inner
class that got moved to Transform) with an accessor interface thrown in
for good measure? Just to change the name? That could be done by using
a source rename rather than gutting the old file and creating the new
one from scratch...? And we don't need both of them, do we?
Transform.java, line 2172 & 2187 - is there a reason to forward to the
factory rather than just instantiating directly?
Transform.java, line 2177 & 2189 - I suppose the reason that the
arguments are not declared to be Immutable in the first place is that
some of these are used from other packages that have no visibility to
the Immutable. I was going to say that we should protect against them
not being Immutables, but I guess these uses are hidden so it doesn't
really affect anything by doing the cast - but it seems sloppy when
looking at the code without looking everywhere to see how they are used
(which I suppose is short-hand for "someone in the future might mess up
because it's not obvious that these can't deal with any other type").
TransformHelper.java, line 108 & 112 - [formatting] perhaps move the
m[xyz],tx to the next line for consistency between the two (like 68 & 76)?
The implementation of ImmutableTransform copies a fair bit of internals
from Affine, but it could just wrap an Affine and save itself all of
that work. Arguably, we could make it a super-class of Affine and then
it would own all of that code and Affine would subclass only for the
modification methods, but we'd either have to expose it as a public
class or have an unexplained inaccessible superclass on Affine...?
(Also, it's not clear if public methods on a non-public superclass of
Affine would be visible through Affine so Affine would either have to
have forwarding methods or we'd have to make the super class public).
On the other hand, what would be wrong with having a public BaseAffine
that was Affine without its modification methods, and having Affine
subclass it? It would be effectively Immutable, but using the word
Immutable in its name might sound like a promise which would be violated
by its subclass - better to leave that property implied by its lack of
mutating methods and possibly mentioning the intent in the javadoc...
...jim
On 5/3/2016 11:24 PM, Chien Yang wrote:
Kevin and Jim,
Please review the proposed fix:
Webrev: https://bugs.openjdk.java.net/browse/JDK-8155762
JIRA: http://cr.openjdk.java.net/~ckyang/JDK-8155762/webrev.00/
Thanks,
- Chien
