scala/collection-strawman

THIS ROOM IS CLOSED

Lukas Rytz

@lrytz

ArrayOps.sorted looks quite dubious to me, it probably performs really badly on primitive arrays. Should we really have that method?

https://github.com/scala/scala/blob/v2.13.0-M5/src/library/scala/collection/ArrayOps.scala#L423-L447

Stefan Zeiger

@szeiger

@lrytz It's probably better to use Sorting.sorted for primitive arrays

Harrison Houghton

@hrhino

couldn't ArrayOps delegate to Sorting?

Stefan Zeiger

@szeiger

Yes, that's what I meant

Harrison Houghton

@hrhino

so not to be a wet blanket or anything, and scala/scala#3434 notwithstanding

Welcome to Scala 2.13.0-M5 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_161).
Type in expressions for evaluation. Or try :help.

scala> val map = collection.mutable.AnyRefMap.empty[String, Int]
map: scala.collection.mutable.AnyRefMap[String,Int] = AnyRefMap()

scala> map.getOrElseUpdate("FB", { map.put("Ea", 1) ; 2 })
res0: Int = 2

scala> map.size
res1: Int = 2

scala> map.toList
res2: List[(String, Int)] = List((FB,2))

scala> map.get("Ea")
res3: Option[Int] = None

is there a reason we can't just say "modifying the map in the closure argument of getOrElseUpdate is not allowed"?

The fix would basically come down to reducing getOrElseUpdate to get(key).getOrElse(put(key, default)), and my understanding of the point of getOrElseUpdate is that it's supposed to be faster than that (due to only having to find the entry once)

(of course, that's not to discount someone cleverer knowing a cleverer fix)

Harrison Houghton

@hrhino

A possible cleverer solution would be to have a modification count.

Seth Tisue

@SethTisue

@hrhino I wouldn’t have been surprised (or disappointed) if scala/bug#8213 had simply been closed as “wontfix"

Harrison Houghton

@hrhino

:+1: agreed. I guess, having fixed it, it's too late to unfix it in 2.13.

I'll see about using a modification count or something

Harrison Houghton

@hrhino

I was looking into some kind of upsert operation for mutable maps and realized it'd have the exact same issue (as 8213) unless I implemented it the slow way.

Seth Tisue

@SethTisue

I guess, having fixed it, it's too late to unfix it in 2.13

idk, you could see what @Ichoran thinks

I put a comment on scala/bug#8213

Harrison Houghton

@hrhino

thanks.

I think the argument is strengthened also by the fact that these aren't the concurrent collections; they break in amusing ways if two methods are "active" on them at the same time (because of concurrency), so it's not a reach IMO to say that breaking if two methods are "active" on the same thread is consistent with the use case.

Ichoran

@Ichoran

I think fixing the behavior is the right thing to do, as explained in my comment on the ticket.

Nth/April

@NthPortal

in practice, do we have any collections (excluding doing bad things with views) where knownSize is not constant time?

e.g. logarithmic or something

Josh

@joshlemer

In general is there a way to write tests that verifies this? i.e. assert that some code completes in < X cpu cycles or something

especially because it's not even enough for knownSize to be O(1), it also must be very fast

Nth/April

@NthPortal

O(log n) can be fast

Harrison Houghton

@hrhino

It can be effectively constant, even.

hrhino @hrhino ducks

Ichoran

@Ichoran

I think knownSize is always O(1) in the library so far. I would say O(log n) is more like O(1) than like O(n) (in the sense that O(log n)^2 is less than O(n)), so if it happened somehow to take O(log n) time to compute knownSize it probably should be computed instead of returning -1.

@joshlemer - No, counting CPU cycles on the JVM is approximately hopeless. You could do a benchmark.

Nth/April

@NthPortal

I kind of feel like the LazyList laziness tests should go in their own file, so that they don't overwhelm all the rest of the LazyList tests

Odd Möller

@odd

How do I get the last element of an IterableOnce or an Iteratorwithout looping myself? Wouldn't last/lastOption make sense for them (I know the other right-oriented variants takeRight/dropRight were removed earlier from the strawman as they would require buffering, but last/lastOption would only need a buffer of size one)?

Julien Richard-Foy

@julienrf

@odd We should probably add last and lastOption to BufferedIterator

Otherwise: ListBuffer.from(iterableOnce).lastOption

Odd Möller

@odd

@julienrf Thanks. Wouldn't that buffer the entire iterable-once during the iteration though?
reduceLeft((_, e) => e) is an alternative I guess.

Julien Richard-Foy

@julienrf

BufferedIterator has a buffer of size 1 only

True, reduce and fold also work

Odd Möller

@odd

@julienrf I see. Perhaps BufferedIterator could be extended to support specifying a longer buffer length and then it could implement the right-oriented operations by using that buffer (would only work for terminating ones and only up to the buffer length naturally)?

Julien Richard-Foy

@julienrf

You can submit the idea… My opinion is that the current behavior of BufferedIterator is fine.

Odd Möller

@odd

appending to todo-list (which took a while since it is quite long, should have used a todo-queue instead...)

Ichoran

@Ichoran

I also think BufferedIterator is fine as-is.

Harrison Houghton

@hrhino

So why are there both concat and appendedAll?

In List for instance, Seq wants concat to mean appendedAll, but there's a StrictOptimizedIterableOps#concat that gets mixed in later..

Julien Richard-Foy

@julienrf

concat comes from Iterable, whose elements have no order, whereas appendedAll comes from Seq and ensures that the elements are concatenated at the end of the lhs collection.

Harrison Houghton

@hrhino

Hmm. So I'll check again, but I think List's concat isn't its appendedAll after all (it gets it from StrictOptimizedIterableOps).

(also, I think I'd assume that "concatenate" implies an order, but the paint's dry on that shed, I'm sure)

Odd Möller

@odd

@hrhino see scala/bug#11128 and scala/scala#7166

The PR (scala/scala#7166) does not pass yet though. Will have time to look into why during the weekend hopefully.

Odd Möller

@odd

@hrhino Also, an additional reason to have appendedAll on Seq in addition to concat is for symmetry with prependedAll (like we have for appended/prepended).

nafg

@nafg

Wouldn't unordered combine be union?

Ichoran

@Ichoran

It's only union on sets.

Where communities thrive

People

Repo info

Activity