Skip to content

length method redundant and missing implementations, and LinkedList's non-tail recursive's length #3996

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

Closed
scabug opened this issue Nov 13, 2010 · 5 comments
Assignees

Comments

@scabug
Copy link

scabug commented Nov 13, 2010

LinkedListLike's length implementation (with which size happens to be overridden) is recursive, but not tail-recursive. Naturally, this means a call to length can result in Stack Overflow.

In a brief research into length and size implementations I couldn't find any other suffering from this malady. So I propose the following implementation for it:

override def length: Int = {
  var len = 0
  var left = this
  while (!left.isEmpty) {
    len += 1
    left = left.next
  }
  len
}

Which happens to be an almost verbattim copy of... I think Stream, replacing tail with next. LinearSeqOptimized also has a very similar definition, but using these instead of left as the variable name, so I'm not sure why Stream is overridding it at all, except, perhaps, as an optimization opportunity missed.

Since tail is present all the way back in Seq, I don't see why the above implementation (but using tail) isn't made default at SeqLike, and let collections which can optimize it override that.

At any rate, LinkedListLike's implementation ought to be fixed.

@scabug
Copy link
Author

scabug commented Nov 13, 2010

Imported From: https://issues.scala-lang.org/browse/SI-3996?orig=1
Reporter: @dcsobral

@scabug
Copy link
Author

scabug commented Nov 13, 2010

@paulp said:
It's worse than that. Here is TraversableOnce:

def size: Int = {
  var result = 0	
  for (x <- self) result += 1
  result
}

Why is this not sufficient. Does someone have evidence that some other way of writing this is faster if you have to traverse the collection regardless? Because otherwise we could just stop overriding it without a clear reason.

@scabug
Copy link
Author

scabug commented Nov 13, 2010

@dcsobral said:
Well, I did see that at TraversableOnce, but it uses a closure, which the implementation using while and tail avoids. I guess both come pretty close with specialization and JVM optimizations, but...

@scabug
Copy link
Author

scabug commented Nov 17, 2010

@axel22 said:
Tail-recursive and loop-based solution seem to have the same performance.

http://www.copypastecode.com/51823/

Tail-recursive is probably more elegant.

@scabug
Copy link
Author

scabug commented Nov 17, 2010

@axel22 said:
(In r23533) Fixes #3996.

No review.

@scabug scabug closed this as completed May 18, 2011
@retronym retronym changed the title length method redundant and missing implementations, and {{LinkedList}}'s non-tail recursive's length length method redundant and missing implementations, and LinkedList's non-tail recursive's length Apr 24, 2017
@retronym retronym changed the title length method redundant and missing implementations, and LinkedList's non-tail recursive's length length method redundant and missing implementations, and LinkedList's non-tail recursive's length Apr 24, 2017
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