hg: valhalla/valhalla/langtools: Add some functional juice to javac code
maurizio.cimadamore at oracle.com
Tue Aug 12 14:02:13 UTC 2014
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,
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
Regarding subtyping vs. flat - main motivation here:
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() = 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
> - 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.
I guess most of this will go away with the factory function.
> - 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
> 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:
> 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
>> 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