amaembo / streamex

Enhancing Java Stream API
Apache License 2.0
2.2k stars 251 forks source link

Examine the possibility to replace Stream<X> with BaseStream<X, ?> in method parameters #76

Open amaembo opened 8 years ago

amaembo commented 8 years ago

It's possible to change some StreamEx method signatures to avoid explicit boxing call for parameter:

append(Stream<? extends T>) -> append(BaseStream<? extends T, ?>)
prepend(Stream<? extends T>) -> prepend(BaseStream<? extends T, ?>)

[Implemented] zipWith(Stream<V>[, BiFunction]) -> zipWith(BaseStream<V, ?>[, BiFunction])

headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends Stream<R>>) ->
    headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends BaseStream<R, ?>>)
headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends Stream<R>> mapper,
        Supplier<? extends Stream<R>> supplier) ->
    headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends BaseStream<R, ?>> mapper,
            Supplier<? extends BaseStream<R, ?>> supplier)

Also MoreCollectors method:

flatMapping(Function<? super T, ? extends Stream<? extends U>> mapper
        [, Collector<? super U, A, R> downstream]) ->
    flatMapping(Function<? super T, ? extends BaseStream<? extends U, ?>> mapper
            [, Collector<? super U, A, R> downstream])

This change will allow to skip .boxed() call in some cases. E.g.:

StreamEx.of("a", "b", "c").zipWith(IntStreamEx.ints())...

This change breaks binary compatibility (recompilation will be necessary), so cannot be introduced in patch release.

lukaseder commented 8 years ago

This change breaks binary compatibility (recompilation will be necessary), so cannot be introduced in minor release.

Hmm, interesting... Personally, I think that breaking binary compatibility between minor releases is OK. After all, I don't see why anyone would just patch a library, incrementing minor releases in a productive build without re-running all build/integration testing, etc.

In this case, though: Why not just overload the methods? Chances are, you're going to eventually overload them with variants that accept Iterator<? super T> and/or Iterable<? super T>, and even T...

amaembo commented 8 years ago

As for append and prepend: they are already overloaded with T... and Collection<T>. The headTail and flatMapping methods could not be overloaded due to the same erasure (actually for these methods binary compatibility is not violated). Overloading with Iterable<T> is bad: this may break source compatibility as StreamEx extends Iterable, so ambiguity will be introduced. Having overloads like append(Stream) and append(BaseStream) looks redundant and causes the bloat. Here I would better sacrifice the binary compatibility than to add an overload.

I still think that the last version digit (patch) updates should guarantee binary/source compatibility to some extent. Though according to semantic versioning I can do whatever I want before version 1.0.0. Here I just mean that this change should be introduced in 0.6.0, not in 0.5.6 (there will be no 0.5.6 actually).

lukaseder commented 8 years ago

Overloading with Iterable is bad: this may break source compatibility as StreamEx extends Iterable, so ambiguity will be introduced.

Yes, I've noticed the same thing in jOOλ. Solution: Overload it 3 times: x(Stream), x(Seq), x(Iterable). This way, there will always be a most-specific type for the compiler to pick.

I still think that the last version digit (patch) updates should guarantee binary/source compatibility to some extent

I completely agree. Although, you said "minor" before editing. Patch releases should remain binary compatible at all costs.

amaembo commented 8 years ago

This still makes problems for those folks who tries to live in both worlds (I don't know for sure, but probably they exist!): Seq.of(1,2,3).append(StreamEx.of(4,5,6)) fails to compile but StreamEx.of(4,5,6).append(Seq.of(1,2,3)) works currently.

Although, you said "minor" before editing

My bad.

lukaseder commented 8 years ago

This still makes problems for those folks who tries to live in both worlds

Well ;-) Just kidding. I haven't thought of this, and you're right. I suspect the source of this confusion is the fact that Stream is not Iterable. I wish it were, but apparently there is some forward compatibility issue with Valhalla.