D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 8085 - std.algorithm.joiner makes invalid assumptions about front()
Summary: std.algorithm.joiner makes invalid assumptions about front()
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: phobos (show other issues)
Version: D2
Hardware: All All
: P2 normal
Assignee: No Owner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-11 12:05 UTC by Walter Bright
Modified: 2012-12-17 12:56 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Walter Bright 2012-05-11 12:05:54 UTC
Given the program:

=========================
import std.stdio;
import std.ascii;
import std.algorithm;
import std.string;

void main() {
//stdin.byChunk(1024).joiner().map!(a =>
toUpper(a)).copy(stdout.lockingTextWriter());

stdin.byLine(KeepTerminator.yes).joiner().map!(a =>
toUpper(a)).copy(stdout.lockingTextWriter());

//stdin.byLine(KeepTerminator.yes).joiner().copy(stdout.lockingTextWriter());

//stdin.byLine(KeepTerminator.yes).copy(stdout.lockingTextWriter());
}
==============================
Compile & run with:

foo <foo.d

trying one of the 4 versions. Versions 1 and 4 work, 2 and 3 fail
horribly.

The output is all scrambled, like this:

IMPORT STD.ASCII;
IMPORT STD.ALGORITIMPORT STD.STRING;
M;

MPORT STD.STRING;
VVOID MAIN() {
STDIN.BYLINE(KEEPTERMINATOR.YES).JOINER().MAP!(A =>
TOUPPER(A)).COPY(STDOUT.LOCKINGTEXT //STDIN.B
YLINE(KEEPTERMINATOR.YES).JOINER().COPY(STDOUT.LOCKINGTEXTWRITER());
CKINGTEXTWRITER());
//STDIN.BYLINE(KEEPTERMINATOR.YES).COPY(STDOUT.LOCKINGTEXTWRITER());
ITER());
}
//STDIN.BYLINE(KEEPTERMINATOR.YES).COPY(STDOUT.LOCKINGTEXTWRITER());
} 

Analysis from Andrei:

Go to algorithm.d line 2370, when joiner() is defined. Then go down to method prepare(). That method calls _items.front.empty, i.e. it assumes _items.front "works" but at the same time saves _current. At the moment _items.front is called, _current gets overwritten.
The code should be changed to not assume that _items.front is independent from _current.
Comment 1 jens.k.mueller 2012-07-06 13:01:33 UTC
I'm thinking about fixing this issue but there are several approaches:

1. Fix joiner (BTW why name it joiner and not just join; same for splitter; Is this because there used to be a string.split or and std.path.join?)

2. Fix ByLine to not invalidate front when doing a popFront
Is it common in Phobos for a range to invalidate front? ByLine and probably also ByChunk (haven't checked) are the only ranges that invalidate front, aren't they. There is also ByRecord which does not show up in the documentation.

3. If it is a common scheme to invalidate front then one could implement an adapter to store a copy of front even though that is less efficient.

I prefer option 2 because it can be implemented with little space overhead but you seem to prefer option 1 which suggests that a range *usually* invalidates front.
Comment 2 hsteoh 2012-10-27 13:34:22 UTC
I think that Phobos code, where possible, should not depend on .front not being invalidated, whether or not that's a common occurrence; that way it will work correctly for more range types.
Comment 3 jens.k.mueller 2012-10-28 03:50:16 UTC
That is also Andrei's stand point. There was a discussion about it on the newsgroup recently. But I'm not sure that this is the consensus.
Comment 5 github-bugzilla 2012-12-17 09:08:49 UTC
Commit pushed to master at https://github.com/D-Programming-Language/phobos

https://github.com/D-Programming-Language/phobos/commit/baf5737beb196ba84f16440ab53a191a6958e5aa
Merge pull request #987 from quickfur/new_joiner

Fix issue 8085