https://issues.dlang.org/show_bug.cgi?id=15293

--- Comment #10 from HeiHon <heiko.honrath@web.de> ---
(In reply to Steven Schveighoffer from comment #9)
> When it was originally written, this is what byLine did, but now it's changed.

So probably the change to byLine introduced the problem?

I tested my examples on 2.068.2 and some versions before. No problem there on Windows.

Also no problem with byLineCopy - neither with 2.068.2 nor 2.069.

--

https://issues.dlang.org/show_bug.cgi?id=15293

--- Comment #11 from Steven Schveighoffer <schveiguy@yahoo.com> ---
(In reply to HeiHon from comment #10)
> (In reply to Steven Schveighoffer from comment #9)
> > When it was originally written, this is what byLine did, but now it's changed.
> 
> So probably the change to byLine introduced the problem?

No, what I meant by that is that byLine simply called readln over and over with the same buffer. So killing the performance of this mechanism would kill the performance of byLine.

However, now byLine always passes in the same buffer without caring what readln did. So the concern that made this whole thing complicated sort of went away :)

> I tested my examples on 2.068.2 and some versions before. No problem there on Windows.

The readln updates were released in 2.069, so this makes sense.

--

https://issues.dlang.org/show_bug.cgi?id=15293

--- Comment #12 from ag0aep6g@gmail.com ---
(In reply to Steven Schveighoffer from comment #9)
> The reason it was done this way is to allow for a common usage of readln (namely, reading into a buffer over and over again) to continue to have good performance.

I understand that, and I think that goal is not compatible with having sane behavior. In my opinion, calling assumeSafeAppend on a smaller slice is not acceptable when larger slices may exist in the outside world.

> If we took the "safe" route (and believe me, it was debated and considered), then this code would reallocate every time a larger line was read. When it was originally written, this is what byLine did, but now it's changed.
> 
> > The other readlnImpl variants don't work like that
> 
> looking at it now, the other variants do exactly as I said above -- they reallocate when the line is longer.

Which is simply the right thing to do. With the API of readln I think we just have to accept the allocations.

> So technically we could move to this, we just have to accept a drop in performance for the code.
> 
> Have you tested the performance of your new code vs. the current implementation?

No. I'm sure there is going to be a performance drop. I don't think that it matters, though. Correctness beats speed.

It also only affects naive readln calls on Windows. byLine is already cautious about what exactly it gives readln as a buffer, so it shouldn't get slower. And on other OSs readnl already does the excessive allocations.

--

https://issues.dlang.org/show_bug.cgi?id=15293

--- Comment #13 from Steven Schveighoffer <schveiguy@yahoo.com> ---
(In reply to ag0aep6g from comment #12)

> It also only affects naive readln calls on Windows. byLine is already cautious about what exactly it gives readln as a buffer, so it shouldn't get slower. And on other OSs readnl already does the excessive allocations.

I think you're right. It seems that the naive loop is simply bad code, and we shouldn't cater to it. It also makes using a stack buffer as a starting point very unwieldy. Since byLine is no longer affected by such a change, I think we should do it. There is already an example in the code to show how to properly use readln in a loop (added as a result of the previous update).

Thanks

--

https://issues.dlang.org/show_bug.cgi?id=15293

--- Comment #14 from github-bugzilla@puremagic.com ---
Commits pushed to stable at https://github.com/D-Programming-Language/phobos

https://github.com/D-Programming-Language/phobos/commit/994d6b81815bc70ab8507ece7285b49ef5ce6d2d fix issue 15293

ReadlnAppender tried to claim the capacity of the passed buffer, calling assumeSafeAppend on the result so that on the next call it has a capacity again that can be claimed.

The obvious problem with that: readln would stomp over memory that it has not been given.

There was also a subtler problem with it (which caused issue 15293): When readln wasn't called with the previous line, but with the original buffer (byLine does that), then the passed buffer had no capacity, so ReadlnAppender would not assumeSafeAppend when slicing the new line from it. But without a new assumeSafeAppend, the last one would still be in effect, possibly on a sub slice of the new line.

https://github.com/D-Programming-Language/phobos/commit/fc77dbbfa93d126c5dfec7c03cc8939b819c09a9 Merge pull request #3802 from aG0aep6G/15293

fix issue 15293

--

https://issues.dlang.org/show_bug.cgi?id=15293

--- Comment #15 from github-bugzilla@puremagic.com ---
Commits pushed to master at https://github.com/D-Programming-Language/phobos

https://github.com/D-Programming-Language/phobos/commit/994d6b81815bc70ab8507ece7285b49ef5ce6d2d fix issue 15293

https://github.com/D-Programming-Language/phobos/commit/fc77dbbfa93d126c5dfec7c03cc8939b819c09a9 Merge pull request #3802 from aG0aep6G/15293

--