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

Remi Forax forax at univ-mlv.fr
Tue Aug 12 18:10:49 UTC 2014


On 08/12/2014 04:02 PM, Maurizio Cimadamore wrote:
> 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.

javac code source should be hackable easily, so anybody can implement 
it's own extension that maybe one day will be a Java feature.
so the code has to be simple and lean.

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

sorry, bold comment from me.

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

Liskov is about types and behaviors, there is no field in Liskov world.
I think that if Tuple3 is a subtype of Tuple2, it will hide some bugs 
instead of allowing polymorphic code.
Mapping a Tuple3 to a Tuple2 by explicitly discarding one arguments is 
IMO better.
   stream.map(tuple3 -> new Tuple2<>(tuple3.item0, tuple3.item1)). ...
and again, tuples are value types for me; if you want subtyping, you 
should not use class inheritance
but tuple implementation should implement a hierarchy of interfaces.

>
> 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 :-)

see above :)

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

Speaking of perf, equals and hashCode implementation should not be shared,
I believe that Hotspot will not be able to optimize the call to get() 
inside them because equals and hashCode are usually
called from callsites that are megamorphic and can not rely on profiling 
since one method is used for several tuple implementations.

So i don't think it makes a perf difference with the current code,
it's just that I find it more readable.

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

That's why Italian or French keyboard are better, '*' and '&' are at 
both side of the keyboard :)

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



More information about the valhalla-dev mailing list