hg: valhalla/valhalla/langtools: Add some functional juice to javac code
forax at univ-mlv.fr
Tue Aug 12 12:04:04 UTC 2014
There are some design issues and some implementation bugs in your
implementation of tuples
First, you have a *public* base class (Tuple) that provide generic
equals, hashCode and toString,
then Tuple2 inherits from Tuple, tuple3 inherits from Tuple2, etc.
There is no need to make Tuple public, the type tuple is useless outside
of the different tuple
implementations and having Tuple3 a subtype of Tuple2 is IMO dubious
I see 3 reasons to not have a deep linear hierarchy but a flat hierarchy
with each tuple implementation
that inherits from Tuple :
- inheritance implies subtyping and I don't think that subtyping
between Tuple3 a,d Tuple2 is a good idea.
- tuple implementations should be final to be truly immutable (and
retrofitable to value type, later)
- VMs do not like deep inheritance tree.
The way the code is generified, is just plain wrong, most of the code is
by example Tuple.get or Builder.makeTuple uses the target type in a way
that can not be safe.
This is clearly wrong, we are supposed to use
@SuppressWarnings("unchecked") only when
the code is safe but the compiler is not able to understand that and not
because we hope
the caller of the code to do the *right* thing.
Now, method by method,
- Tuple.get, should return an Object, there is no need to try to infer a
type here given
it will be used in equals, hashCode and toString that are methods
that exist on java.lang.Object.
- Tuple.indices, there is no need to return all indices, the size of the
tuple is enough, one can use
IntStream.range(0, last_index + 1) to generate all indices. Moreover,
because there is not yet
a frozen array in Java, indices should do a defensive copies,
otherwise an implementation can write
tuple.indices() = 128;
- equals, instead of checking indices length, I think it's better to
check if this.getClass() == obj.getClass().
- hashCode, I don't understand how this code can lead to a good
hashCode, are you sure you whant
to use a boolean '&' because basically you only select two bits (17
== 16 + 1) of each object hashCode.
- toString, the two calls to map() can be grouped in one and joining()
has an overloads that takes
a prefix and a suffix.
- zip, no exception if the lists doesn't has the same size, transferring
the values of a tuple to a List (linked)
before to the tuple seems wrong algorithmically.
- class Builder, there is a confusion in the code between currPos and
so include and makeTuple doesn't work as expected.
- TupleX, the constructor should check that the arguments can not be null,
otherwise equals and hashCode will throw a NPE.
Here is a version that fix most of the issues:
it uses a java.util.List instead of a javac List, easier for me to test,
so it need a little adaptation.
On 08/11/2014 06:38 PM, maurizio.cimadamore at oracle.com wrote:
> Changeset: 98cd669dfb21
> Author: mcimadamore
> Date: 2014-08-11 17:32 +0100
> URL: http://hg.openjdk.java.net/valhalla/valhalla/langtools/rev/98cd669dfb21
> Add some functional juice to javac code
> *) Added new collector function to List
> *) Replace javac's Pair with more general Tuple-like class(es)
> ! src/share/classes/com/sun/tools/javac/api/JavacTrees.java
> ! src/share/classes/com/sun/tools/javac/code/Attribute.java
> ! src/share/classes/com/sun/tools/javac/code/Lint.java
> ! src/share/classes/com/sun/tools/javac/code/Types.java
> ! src/share/classes/com/sun/tools/javac/comp/Annotate.java
> ! src/share/classes/com/sun/tools/javac/comp/Attr.java
> ! src/share/classes/com/sun/tools/javac/comp/Check.java
> ! src/share/classes/com/sun/tools/javac/comp/Infer.java
> ! src/share/classes/com/sun/tools/javac/comp/Resolve.java
> ! src/share/classes/com/sun/tools/javac/comp/SpecializeTypes.java
> ! src/share/classes/com/sun/tools/javac/jvm/ClassReader.java
> ! src/share/classes/com/sun/tools/javac/jvm/ClassWriter.java
> ! src/share/classes/com/sun/tools/javac/jvm/JNIWriter.java
> ! src/share/classes/com/sun/tools/javac/main/JavaCompiler.java
> ! src/share/classes/com/sun/tools/javac/model/AnnotationProxyMaker.java
> ! src/share/classes/com/sun/tools/javac/model/JavacElements.java
> ! src/share/classes/com/sun/tools/javac/processing/JavacMessager.java
> ! src/share/classes/com/sun/tools/javac/sym/CreateSymbols.java
> ! src/share/classes/com/sun/tools/javac/tree/TreeMaker.java
> ! src/share/classes/com/sun/tools/javac/util/List.java
> ! src/share/classes/com/sun/tools/javac/util/Log.java
> - src/share/classes/com/sun/tools/javac/util/Pair.java
> + src/share/classes/com/sun/tools/javac/util/Tuple.java
> ! src/share/classes/com/sun/tools/javadoc/AnnotationDescImpl.java
> ! test/lib/combo/tools/javac/combo/JavacTemplateTestBase.java
> ! test/tools/javac/failover/CheckAttributedTree.java
> ! test/tools/javac/lambdaShapes/org/openjdk/tests/javac/FDTest.java
More information about the valhalla-dev