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

[Maurizio Cimadamore]

While I tend to agree with most of the point below, you do realize that 
this is a javac-only tuple implementation and not a general purpose one, 
right?
I.e. for this reason, I wasn't super worried about unchecked warnings.

Also, you say most of the code is unchecked - which is not true - if you 
remove the get() warnings (which as you pointed out can be avoided by 
having the method returning Object) - then, we are left with a couple of 
unchecked around zip. Which I can probably eliminate by replacing the 
builder with the factory function, similarly to what you have done in 
your code.

Regarding subtyping vs. flat - main motivation here:

http://gbracha.blogspot.co.uk/2007/02/tuples.html

By Liskov, a 3-element tuple is also a 2-element tuple - i.e. it has a 
first and second element. It's a different way of thinking about tuples 
- if you think about them as record types, what I ended up with is 
closer with what the theory says. Maybe we don't care this degree of 
fidelity - but again, this is an internal implementation.

See below for more specific replies:
>
> 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.
good idea - I got here from a weird path
> - 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;

well, this was to avoid calling range all the times. Do we care about 
mutability from implementations that are bound to be javac-specific?
Again it seems like you think this should be a java.util API :-)


> - equals, instead of checking indices length, I think it's better to 
> check if this.getClass() == obj.getClass().
I was torn between the two... is that really that big of a perf difference?

> - 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.

Typo on my part - should have been '*' instead of '&', as in previous 
Pair.hashCode method

> - toString, the two calls to map() can be grouped in one and joining() 
> has an overloads that takes
>   a prefix and a suffix.
Cool, thx
> - 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.
I guess most of this will go away with the factory function.
> - class Builder, there is a confusion in the code between currPos and 
> tupleArray.length,
>   so include and makeTuple doesn't work as expected.
Good point
> - TupleX, the constructor should check that the arguments can not be 
> null,
>   otherwise equals and hashCode will throw a NPE.
again - good idea, but this is not something the old Pair class used to do.
>
> 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.
Thanks

Maurizio
>
> 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
>>
>