New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SI-7445 ListMap.tail is returning wrong result #3388

Merged
merged 1 commit into from Jan 24, 2014

Conversation

Projects
None yet
4 participants
@rklaehn
Contributor

rklaehn commented Jan 20, 2014

Reverted the commit that introduced the bug, and modified HashMap to no
longer assume that tail is O(1).

Review by @Ichoran, @soc

SI-7445 ListMap.tail is returning wrong result
Reverted the commit that introduced the bug, and modified HashMap to no
longer assume that tail is O(1).

Review by @Ichoran, @soc
@soc

This comment has been minimized.

Show comment
Hide comment
@soc

soc Jan 21, 2014

Member

LGTM. Thanks for looking into this, I really couldn't find the time to do it.

Member

soc commented Jan 21, 2014

LGTM. Thanks for looking into this, I really couldn't find the time to do it.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jan 23, 2014

Member

Thank you indeed! What should we do about the docs while we're on the topic?

Member

adriaanm commented Jan 23, 2014

Thank you indeed! What should we do about the docs while we're on the topic?

@rklaehn

This comment has been minimized.

Show comment
Hide comment
@rklaehn

rklaehn Jan 23, 2014

Contributor

You mean documenting the ordering properties of ListMap? We might as well document them since we have to guarantee them in any case.

It's a bit annoying that the behavior of ListSet and ListMap is so different, but I guess we can't change one to match the other.

Are documentation PRs still OK while in the RC phase of a release?

Contributor

rklaehn commented Jan 23, 2014

You mean documenting the ordering properties of ListMap? We might as well document them since we have to guarantee them in any case.

It's a bit annoying that the behavior of ListSet and ListMap is so different, but I guess we can't change one to match the other.

Are documentation PRs still OK while in the RC phase of a release?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jan 23, 2014

Member

Yeah, documentation is always more than ok! :-)

Member

adriaanm commented Jan 23, 2014

Yeah, documentation is always more than ok! :-)

adriaanm added a commit that referenced this pull request Jan 24, 2014

Merge pull request #3388 from rklaehn/issue/7445
 ListMap.tail is returning wrong result

@adriaanm adriaanm merged commit 5dd4633 into scala:master Jan 24, 2014

1 check passed

default pr-scala Took 63 min.
Details

@rklaehn rklaehn deleted the rklaehn:issue/7445 branch Jan 24, 2014

@rklaehn rklaehn restored the rklaehn:issue/7445 branch Jan 24, 2014

@dmanchester

This comment has been minimized.

Show comment
Hide comment
@dmanchester

dmanchester Feb 8, 2014

Hi,

I came across this pull request while researching immutable ListMaps. I had been wondering whether the behavior I had presumed and have observed--that iteration order reflects insertion order--is guaranteed (Scala 2.9.2 output follows):

scala> import scala.collection.immutable._
import scala.collection.immutable._

scala> val map = ListMap("F" -> "Frank", "G" -> "George")
map: scala.collection.immutable.ListMap[java.lang.String,java.lang.String] =
Map(F -> Frank, G -> George)

scala> val map2 = map + ("H" -> "Henry", "I" -> "Ida")
map2: scala.collection.immutable.ListMap[java.lang.String,java.lang.String] =
Map(F -> Frank, G -> George, H -> Henry, I -> Ida)

Based on Rüdiger's comment above, it sounds like that behavior is indeed guaranteed?

dmanchester commented Feb 8, 2014

Hi,

I came across this pull request while researching immutable ListMaps. I had been wondering whether the behavior I had presumed and have observed--that iteration order reflects insertion order--is guaranteed (Scala 2.9.2 output follows):

scala> import scala.collection.immutable._
import scala.collection.immutable._

scala> val map = ListMap("F" -> "Frank", "G" -> "George")
map: scala.collection.immutable.ListMap[java.lang.String,java.lang.String] =
Map(F -> Frank, G -> George)

scala> val map2 = map + ("H" -> "Henry", "I" -> "Ida")
map2: scala.collection.immutable.ListMap[java.lang.String,java.lang.String] =
Map(F -> Frank, G -> George, H -> Henry, I -> Ida)

Based on Rüdiger's comment above, it sounds like that behavior is indeed guaranteed?

@rklaehn

This comment has been minimized.

Show comment
Hide comment
@rklaehn

rklaehn Feb 8, 2014

Contributor

Hi,

Yes, we have to guarantee the that iteration order reflects insertion
order, because many people have come to rely on it, and also ordering
behaviour is one of the few reasons one would use a ListMap in the first
place. (Note that confusingly, in ListSet the ordering of the elements is
the reverse order in which they have been added)

I am currently busy with the day job and the kids, but when I have some
time I will do a PR to update the docs accordingly.

By the way: it's a shame ListMap is called ListMap. If we would back it by
a vector which has quick append, we could guarantee the order without the
large cost of reversing the iterator every time.

On Sat, Feb 8, 2014 at 4:34 AM, dmanchester notifications@github.comwrote:

Hi,

I came across this pull request while researching immutable ListMaps. I
had been wondering whether the behavior I had presumed and have
observed--that iteration order reflects insertion order--is guaranteed
(Scala 2.9.2 output follows):

scala> import scala.collection.immutable._
import scala.collection.immutable._

scala> val map = ListMap("F" -> "Frank", "G" -> "George")

map: scala.collection.immutable.ListMap[java.lang.String,java.lang.String]

Map(F -> Frank, G -> George)

scala> val map2 = map + ("H" -> "Henry", "I" -> "Ida")
map2:
scala.collection.immutable.ListMap[java.lang.String,java.lang.String] =
Map(F -> Frank, G -> George, H -> Henry, I -> Ida)

Based on Rüdiger's comment above, it sounds like that behavior is indeed
guaranteed?

Reply to this email directly or view it on GitHubhttps://github.com/scala/scala/pull/3388#issuecomment-34531167
.

Contributor

rklaehn commented Feb 8, 2014

Hi,

Yes, we have to guarantee the that iteration order reflects insertion
order, because many people have come to rely on it, and also ordering
behaviour is one of the few reasons one would use a ListMap in the first
place. (Note that confusingly, in ListSet the ordering of the elements is
the reverse order in which they have been added)

I am currently busy with the day job and the kids, but when I have some
time I will do a PR to update the docs accordingly.

By the way: it's a shame ListMap is called ListMap. If we would back it by
a vector which has quick append, we could guarantee the order without the
large cost of reversing the iterator every time.

On Sat, Feb 8, 2014 at 4:34 AM, dmanchester notifications@github.comwrote:

Hi,

I came across this pull request while researching immutable ListMaps. I
had been wondering whether the behavior I had presumed and have
observed--that iteration order reflects insertion order--is guaranteed
(Scala 2.9.2 output follows):

scala> import scala.collection.immutable._
import scala.collection.immutable._

scala> val map = ListMap("F" -> "Frank", "G" -> "George")

map: scala.collection.immutable.ListMap[java.lang.String,java.lang.String]

Map(F -> Frank, G -> George)

scala> val map2 = map + ("H" -> "Henry", "I" -> "Ida")
map2:
scala.collection.immutable.ListMap[java.lang.String,java.lang.String] =
Map(F -> Frank, G -> George, H -> Henry, I -> Ida)

Based on Rüdiger's comment above, it sounds like that behavior is indeed
guaranteed?

Reply to this email directly or view it on GitHubhttps://github.com/scala/scala/pull/3388#issuecomment-34531167
.

@dmanchester

This comment has been minimized.

Show comment
Hide comment
@dmanchester

dmanchester Feb 9, 2014

Great--thanks for confirming, Rüdiger.

Regarding the name, might there be support for adding an InsertionOrderedMap and InsertionOrderedSet (or something like that)? The map could take advantage of optimizations like the one you propose, and the set could address ListSet's unexpected ordering.

dmanchester commented Feb 9, 2014

Great--thanks for confirming, Rüdiger.

Regarding the name, might there be support for adding an InsertionOrderedMap and InsertionOrderedSet (or something like that)? The map could take advantage of optimizations like the one you propose, and the set could address ListSet's unexpected ordering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment