Added type parameter to Iterator.empty and test cases

[kelebra]

Transfer of fix in PR for issue #10711 in scala repository as per @szeiger suggestion

Also extended IterableFactory in Iterator's companion object as was suggested in PR above.

[mkeskells]

use assertSame when the results should be eq

[kelebra]

Fixed, thanks for pointing that out.

[mkeskells]

inlining this method has little effect
If the method is inlinable it requires the caller to be monomorphically dispatched, which would imply that you have you pretty redundant code in this case

what you need to do in inline the target access. See rorygraves/scalac_perf#46
maybe @inlinedef empty[T]: Iterator[T] = _empty: @inline
but you will need to check the bytecode :-(
@szeiger can you consider where else this should be applied, or discuss with @retronym if the optimiser should do this automatically

[kelebra]

All implementations below:

@inline final def empty[T]: Iterator[T] = _empty: @inline
@inline final def empty[T]: Iterator[T] = _empty
def empty[T]: Iterator[T] = _empty

result in the following bytecode:

  public static <T> strawman.collection.Iterator<T> empty();
    Code:
       0: getstatic     #66                 // Field strawman/collection/Iterator$.MODULE$:Lstrawman/collection/Iterator$;
       3: invokevirtual #146                // Method strawman/collection/Iterator$.empty:()Lstrawman/collection/Iterator;
       6: areturn

As per client code is concerned I tried definition

@inline final def empty[T]: Iterator[T] = _empty: @inline

and the following client code:

object IteratorInlineTest {

  def inlineTest1: immutable.List[scala.Int] = immutable.List.from(Iterator.empty)

  def inlineTest2(iter: Iterator[scala.Int] = Iterator.empty): Iterator[scala.Int] =
    Iterator(iter, Iterator(3, 21), Iterator(1, 2, 34))
      .foldLeft(Iterator.empty[scala.Int])(_ ++ _)
  
  def inlineTest3: Iterator[scala.Int] = {
    var itr = Iterator.empty[Int]
    itr = itr ++ Iterator(123)
    itr = itr ++ Iterator(321)
    itr
  }
}

which resulted with the following bytecode:

public final class strawman.collection.IteratorInlineTest {
  public static strawman.collection.Iterator<java.lang.Object> inlineTest2$default$1();
    Code:
       0: getstatic     #16                 // Field strawman/collection/IteratorInlineTest$.MODULE$:Lstrawman/collection/IteratorInlineTest$;
       3: invokevirtual #18                 // Method strawman/collection/IteratorInlineTest$.inlineTest2$default$1:()Lstrawman/collection/Iterator;
       6: areturn

  public static strawman.collection.Iterator<java.lang.Object> inlineTest3();
    Code:
       0: getstatic     #16                 // Field strawman/collection/IteratorInlineTest$.MODULE$:Lstrawman/collection/IteratorInlineTest$;
       3: invokevirtual #21                 // Method strawman/collection/IteratorInlineTest$.inlineTest3:()Lstrawman/collection/Iterator;
       6: areturn

  public static strawman.collection.Iterator<java.lang.Object> inlineTest2(strawman.collection.Iterator<java.lang.Object>);
    Code:
       0: getstatic     #16                 // Field strawman/collection/IteratorInlineTest$.MODULE$:Lstrawman/collection/IteratorInlineTest$;
       3: aload_0
       4: invokevirtual #25                 // Method strawman/collection/IteratorInlineTest$.inlineTest2:(Lstrawman/collection/Iterator;)Lstrawman/collection/Iterator;
       7: areturn

  public static strawman.collection.immutable.List<java.lang.Object> inlineTest1();
    Code:
       0: getstatic     #16                 // Field strawman/collection/IteratorInlineTest$.MODULE$:Lstrawman/collection/IteratorInlineTest$;
       3: invokevirtual #29                 // Method strawman/collection/IteratorInlineTest$.inlineTest1:()Lstrawman/collection/immutable/List;
       6: areturn
}

and actually I get the same bytecode in the case of current definition. Please advice if any other case has to be taken into consideration apart from these ones above.

I guess the difference from the example that you provided is the fact that _empty is static.

[julienrf]

I’m ok with the idea of having this polymorphic Iterator.empty method. I’m not sure of the benefits of extending IterableFactory, though.

[kelebra]

Do not shoot the messenger: idea was initially proposed by @szeiger in scala PR.

As per my understanding in terms of public API these two were different only in terms of from(Iterable[A]) and newBuilder[A] methods which were trivial to implement. Given that and the fact that ~21 other companion objects extend it, it might make sense at least from uniformity standpoint.

[julienrf]

Why not assertSame here?

[kelebra]

Missed this one, thanks and fixed.

[kelebra]

Is there anything required to merge this PR?

Test cases were fixed (assertSame usage) and evidence of inlining was provided. I guess it is up to @mkeskells now.

[julienrf]

Thanks!

[mkeskells]

nothing is ever up to me - you overestimate my influence :-)