Skip to content

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented Sep 4, 2012

Add copyInto as discussed on newsgroup.
copyInto should be used to copy InputRanges into OutputRanges. Contrary to std.algorithm.copy the target parameter is passed by reference. This allows OutputRanges which keep state to work (e.g. std.array.Appender, std.digest).

Suggestions are welcome. I also wonder whether the InputRange should be passed by ref or by value?

Test Results

@@ -7,6 +7,7 @@ $(VERSION 060, ddd mm, 2012, =================================================,
Please see the documentation for std.string.format and std.string.sformat for details.))
$(LI std.bitmanip: Added peek, read, write, and append for converting
ranges of bytes to and from integral types.)
$(LI std.algorithm: Added copyInto to copy InputRanges into OutputRanges.)
Copy link
Member

Choose a reason for hiding this comment

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

  1. This is in the wrong portion of the changelog. It needs to go in 2.061, not 2.060.
  2. In general, if you change the changelog as part of a pull request like this, you're going to merge conflicts very easily. IMHO, it's generally better to do them as a separate request, though that does increase the risk of them not making it into the changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Seems I have confused the version numbers :-( Should be fixed now.
  2. I'll keep that in mind for future pull requests.

copyInto should be used to copy InputRanges into
OutputRanges. Contrary to std.algorithm.copy the
target parameter is passed by reference. This
allows OutputRanges which keep state to work
(e.g. std.array.Appender, std.digest).
$(WEB sgi.com/tech/stl/copy_backward.html, STL's copy_backward) is
needed, use $(D copy(retro(source), retro(target))). See also $(XREF
range, retro). For arrays, see also $(LREF copy).

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused. Much of the comment above refers to copy(), whereas the function definition is copyInto(). Also, what is the difference between copy and copyInto? Can't we make copy accommodate the functionality of copyInto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The description was copy/pasted from copy and it seems I forgot to rename some occurrences of 'copy'.
The difference between copyInto and copy is that the OutputRange is passed by reference for copyInto.

This solves the std.digest issue related to copy ( http://forum.dlang.org/post/503A3CB8.607@erdani.org )

Merging this functionality into copy is imho the better option, but it could break some corner cases:

struct Value
{
    int a;
    void put(int x)
    {
        a += x;
    }
}

auto val = Value(10);
auto res = copy([1,2,3], val);
auto res2 = copy([4,5], val);
assert(val.a == 10); //With proposed changes: 25
assert(res.a == 16); //With proposed changes: 25
assert(res2.a == 19); //With proposed changes: 25

The documentation for copy doesn't describe how it works with OutputRanges so we can probably make that change?

Another question is whether we want to change the behaviour for arrays as well and pass arrays by reference, so their .length would get updated?

@andralex
Copy link
Member

I think we should go with changing copy.

@jpf91 jpf91 closed this Sep 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants