hg: valhalla/valhalla/langtools: Add some functional juice to javac code

[Remi Forax]

Hi Maurizio,
There are some design issues and some implementation bugs in your 
implementation of tuples
(http://hg.openjdk.java.net/valhalla/valhalla/langtools/file/98cd669dfb21/src/share/classes/com/sun/tools/javac/util/Tuple.java).

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 
unsafe,
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()[0] = 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 
tupleArray.length,
   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:
   https://gist.github.com/forax/6211ceace9f31c8989bf
it uses a java.util.List instead of a javac List, easier for me to test, 
so it need a little adaptation.

cheers,
Rémi

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
>