Skip to content
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

Vector updated, +:, and :+ ignore provided builder, breaking type safety #5937

Closed
scabug opened this issue Jun 17, 2012 · 2 comments
Closed

Comments

@scabug
Copy link

scabug commented Jun 17, 2012

The updated, +:, and :+ methods ignore the provided CanBuildFrom instance, likely for performance reasons. This breaks type safety:

scala> (Vector(1,2,3) :+ 4)(collection.breakOut):List[Int]
java.lang.ClassCastException: scala.collection.immutable.Vector cannot be cast to scala.collection.immutable.List

The implementation of List.+: gets it right though, with minimal overhead (one instanceof check). I've attached a diff that applies the same implementation to Vector. All tests pass when applied to current trunk (277dc7cf43566f8294bde4143107d9bfaa59e8e3)

Obviously, performance may be affected, although an instanceof check should be cheap compared to the required node allocation, etc. The original code seems to have been added on 2009-10-14 with commit 1747692434cece862d63a0f67decd810707b1508 and includes the comment: TODO: check overhead of builder factories for methods updated, \+: and :\+. The comment was removed when the code was moved from IndexedSeq to Vector (commit 6f7723bea494da2616edc1877d2402d356787512). I wonder if any overhead checking was ever done.

@scabug
Copy link
Author

scabug commented Jun 17, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5937?orig=1
Reporter: Erik Rozendaal (erikrozendaal)
Affected Versions: 2.9.2, 2.10.0
Attachments:

  • vector.diff (created on Jun 17, 2012 4:24:49 PM UTC, 1699 bytes)

@scabug scabug closed this as completed Jul 18, 2012
@scabug
Copy link
Author

scabug commented Jul 18, 2012

@adriaanm said:
scala/scala#940

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

No branches or pull requests

2 participants