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.

Sign up for GitHub

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

Jump to bottom

issue8851 #2533

Merged
merged 1 commit into from Sep 19, 2014
Merged

issue8851 #2533

merged 1 commit into from Sep 19, 2014

Conversation

burner
Copy link
Member

@burner burner commented Sep 18, 2014

quickfur
quickfur reviewed Sep 19, 2014
View reviewed changes
std/array.d
resultLen += temp.front.length + sepArrLength;
resultLen -= sepArrLength;
result.reserve(resultLen);
version(unittest) scope(exit) assert(result.data.length == resultLen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanity checking

@quickfur
Copy link
Member

Looks OK to me, though I wonder if we should put a generic version in std.algorithm instead, and just make this function in std.array a thin wrapper around that.

monarchdodra
monarchdodra reviewed Sep 19, 2014
View reviewed changes
std/array.d
@@ -1684,6 +1683,53 @@ ElementEncodingType!(ElementType!RoR)[] join(RoR, R)(RoR ror, R sep)
}

/// Ditto
ElementEncodingType!(ElementType!RoR)[] join(RoR, R)(RoR ror, R sep)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change R for E, since sep is of type "element".

@monarchdodra
Copy link
Collaborator

Looks OK to me, though I wonder if we should put a generic version in std.algorithm instead, and just make this function in std.array a thin wrapper around that.

joiner is in algorithm. join is in array (because it produces and allocates an array). join is not implemented in terms of joiner because (afaiik) of performance reasons: join will eagerly evaluate its length, whereas joiner will not report its length. Also, joiner operates on ElementType, since it is lazy, whereas join can return a narrow string (with potentially no endecoding overhead).

monarchdodra
monarchdodra reviewed Sep 19, 2014
View reviewed changes
std/array.d
&& !is(RetTypeElement == dchar))
{
RetTypeElement[4] encodeSpace;
immutable sepArrLength = encode(encodeSpace, to!dchar(sep));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nitpick, but is this "to" necessary? It adds a bounds check overhead, which should be covered in encode anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was not necessary, thanks for catching that

@burner
issue8851
082176b 
some more

some review fixes

some more

removed a to
@quickfur
Copy link
Member

@monarchdodra Fair enough. Looks good to me.

@quickfur
Copy link
Member

Auto-merge toggled on

quickfur pushed a commit that referenced this pull request Sep 19, 2014
Merge pull request #2533 from burner/issue8851
6e626e0 
issue8851
@quickfur quickfur merged commit 6e626e0 into dlang:master Sep 19, 2014
@burner
Copy link
Member Author

burner commented Sep 20, 2014

thanks @quickfur and @monarchdodra

@burner burner deleted the issue8851 branch October 23, 2015 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@burner @quickfur @monarchdodra