Fix String.concat ignoring the len parameter #2723
Conversation
The loop pushing values from iter didn't increment n, resulting in finite iterators getting concated completely. Also infinite iterators such as `Iter.repeat_value` would result in the function never returning and eating up memory.
packages/builtin/string.pony
Outdated
| @@ -880,12 +880,14 @@ actor Main | |||
|
|
|||
| n = 0 | |||
|
|
|||
| while n < len do | |||
| while (n < len) or (len == USize(-1)) do | |||
There was a problem hiding this comment.
what's the len == USize(-1) case?
There was a problem hiding this comment.
nvm, i went and looked at the surrounding code. given that len == -1 is the default, i suspect the ordering should be reversed. if -1 is the most common, we should test for that first.
There was a problem hiding this comment.
the default value of len is -1, meaning the amount of values to read should not be limited.
but now that you say it, being unsigned, this could be omitted
There was a problem hiding this comment.
ah right. sorry. sick and not quite with it today.
|
@patroclos can you add a test case for this so we don't get a regression? |
packages/builtin_test/_test.pony
Outdated
| fun name(): String => "builtin/String.concat" | ||
|
|
||
| fun apply(h: TestHelper) => | ||
| h.assert_eq[String](recover String .> concat("ABCD".values(), 1, 2) end, "BC") |
There was a problem hiding this comment.
to match with standard library programming style, this should be String.>concat
SeanTAllen
added
the
changelog - fixed
|
Thanks @patroclos. This is an awesome first contribution. I hope it is the first of many. |
The loop pushing values from iter didn't increment n, resulting in finite iterators getting concated completely.
Also infinite iterators such as
Iter.repeat_valuewould result in the function never returning and eating up memory.This behavior contradicts the docstring.