Add copyInto to std.algorithm #772

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@jpf91

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

@jmdavis jmdavis and 1 other commented on an outdated diff Sep 15, 2012
changelog.dd
@@ -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.)
@jmdavis
jmdavis Sep 15, 2012
  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.
@jpf91
jpf91 Sep 16, 2012
  1. Seems I have confused the version numbers :-( Should be fixed now.

  2. I'll keep that in mind for future pull requests.

@jmdavis jmdavis commented on the diff Sep 15, 2012
std/algorithm.d
+----
+import std.array;
+auto app = appender!string();
+immutable(char)[] input = "abcd";
+
+copyInto(input, app);
+input.copyInto(app);
+assert(app.data == "abcdabcd");
+----
+
+To copy at most $(D n) elements from range $(D a) to range $(D b), you
+may want to use $(D copyInto(take(a, n), b)). To copy those elements from
+range $(D a) that satisfy predicate $(D pred) to range $(D b), you may
+want to use $(D copyInto(filter!(pred)(a), b)).
+ */
+auto ref Range2 copyInto(Range1, Range2)(Range1 source, auto ref Range2 target)
@jmdavis
jmdavis Sep 15, 2012

Honestly, I would think that it would be a bug for an output range to be a value type. By their very nature, they need to be passed to functions and be altered by those functions in a manner which affects the original. So, I don't think that the ref should be necessary. If it is, then we need to fix it so that it isn't (e.g. by fixing Appender if it requires it for some reason).

@DmitryOlshansky
DmitryOlshansky Sep 15, 2012

The problem is as usual with arrays as ranges. If I'm not mistaken they serve as output range just fine.
But then when passed to copyInto the changes to ptr+len won't get reflected to the outside.

@jpf91
jpf91 Sep 16, 2012

Theoretically it might make sense that all OutputRanges should be reference types. But the new std.digest ranges are all value types. std.digest is designed to avoid allocations where possible, so you'd want to have those ranges on the stack. And as allocating classes on stack is cumbersome I've used value types / structs.

AFAIK there's no clear statement anywhere that OutputRanges must be reference types. I'm not sure what's the best solution in this case, but currently 'copy' can only be used with a pointer to a Digest struct and that kinda sucks.

@jpf91 jpf91 Add copyInto to std.algorithm
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).
d58a8c2
@andralex andralex commented on the diff Sep 17, 2012
std/algorithm.d
@@ -5976,6 +5978,57 @@ unittest
}
}
+// copy
+/**
+Copies the content of $(D source) into $(D target) where target
+is an OutputRange. See also $(WEB sgi.com/tech/stl/_copy.html, STL's _copy).
+If a behavior similar to
+$(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).
+
@andralex
andralex Sep 17, 2012

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?

@jpf91
jpf91 Sep 17, 2012

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
D Programming Language 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