Skip to content

Fix Issue 15798 - std.algorithm.mutation.copy takes target by value #6039

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

Closed
JackStouffer wants to merge 2 commits into from
Closed

Fix Issue 15798 - std.algorithm.mutation.copy takes target by value #6039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67c4512
Fix Issue 15798 - std.algorithm.mutation.copy takes target by value
JackStouffer Jan 17, 2018
17a0715
Added changelog entry
JackStouffer Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions changelog/copy-auto-ref.dd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
`std.algorithm.mutation.copy` Has Been Modified For `put`-style Output Ranges

With this release, when the target output range in a call to
$(REF copy, std, algorithm, mutation) is any `put` defining output range (and it
doesn't have $(REF_ALTTEXT assignable elements, hasAssignableElements, std, range, primitives)),
the target is taken as `auto ref`. This new overload is also `void` returning
vs the other overloads which return the unfilled portion of the given range
or array.

Before this release, $(REF copy, std, algorithm, mutation) took all
$(REF_ALTTEXT output ranges, isOutputRange, std, range, primitives) by value.
This leads to some issues when output ranges aren't designed around a reference
to heap memory, e.g. the hash ranges in $(MREF std,digest):

-------
// v2.078 and earlier
import std.algorithm.mutation : copy;
import std.digest.digest : toHexString;
import std.digest.md : MD5;
import std.range : put;

auto s = "Hello!\n";
auto h1 = makeDigest!MD5;
auto h2 = makeDigest!MD5;

put(h1, s); // takes h1 by ref
copy(s, h2); // takes h2 by value

assert(h1.finish().toHexString == "E134CED312B3511D88943D57CCD70C83");
// Different result because copy didn't properly insert the contents of s
assert(h2.finish().toHexString == "D41D8CD98F00B204E9800998ECF8427E");
-------

This issue no longer occurs in 2.079 and later as long as `target` is an lvalue.
31 changes: 25 additions & 6 deletions std/algorithm/mutation.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,18 +351,25 @@ private enum bool areCopyCompatibleArrays(T1, T2) =

// copy
/**
Copies the content of `source` into `target` and returns the
remaining (unfilled) part of `target`.
Copies the content of `source` into `target`.

Preconditions: `target` shall have enough room to accommodate
If `target` is an $(REF_ALTTEXT output range, isOutputRange, std,range,primitives)
which is a range with assignable elements, then `copy` returns the remaining
(unfilled) part of `target`. In this case, `target` must have enough room to accommodate
the entirety of `source`.

If `target` is an $(REF_ALTTEXT output range, isOutputRange, std,range,primitives)
which defines a `put` method, than nothing is returned. In this case, it's generally
recommended to pass `target` as an lvalue, to avoid any issues with copying the output
data by value.

Params:
source = an $(REF_ALTTEXT input range, isInputRange, std,range,primitives)
target = an output range
target = an $(REF_ALTTEXT output range, isOutputRange, std,range,primitives)

Returns:
The unfilled part of target
The unfilled part of `target` if `target` is not a `put` defining output
range. `void` otherwise.

See_Also:
$(HTTP sgi.com/tech/stl/_copy.html, STL's _copy)
Expand Down Expand Up @@ -399,7 +406,8 @@ if (areCopyCompatibleArrays!(SourceRange, TargetRange))
TargetRange copy(SourceRange, TargetRange)(SourceRange source, TargetRange target)
if (!areCopyCompatibleArrays!(SourceRange, TargetRange) &&
isInputRange!SourceRange &&
isOutputRange!(TargetRange, ElementType!SourceRange))
isOutputRange!(TargetRange, ElementType!SourceRange) &&
(isArray!TargetRange || hasAssignableElements!TargetRange))
Copy link
Contributor

@wilzbach wilzbach Jan 17, 2018

Choose a reason for hiding this comment

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

Shouldn't this be just hasAssignableElements!TargetRange?

string a;
isArray!(typeof(a)).writeln; // true
isOutputRange!(typeof(a), char).writeln; // false
a[0] = "b"; // cannot modify immutable expression

https://run.dlang.io/is/ivpUfM

Copy link
Member

@schveiguy schveiguy Jan 18, 2018

Choose a reason for hiding this comment

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

Hm.. I think hasAssignableElements would fail on a char[], but not sure if that's a valid output range anyway.

Copy link
Member

@schveiguy schveiguy Jan 18, 2018

Choose a reason for hiding this comment

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

tested, char[] is not an output range for char, which is... strange. It looks like there's special handling for characters in put, so I would have expected this to work.

{
// Specialize for 2 random access ranges.
// Typically 2 random access ranges are faster iterated by common
Expand All @@ -422,6 +430,17 @@ if (!areCopyCompatibleArrays!(SourceRange, TargetRange) &&
}
}

/// ditto
void copy(SourceRange, TargetRange)(SourceRange source, auto ref TargetRange target)
if (!areCopyCompatibleArrays!(SourceRange, TargetRange) &&
isInputRange!SourceRange &&
isOutputRange!(TargetRange, ElementType!SourceRange) &&
!isArray!TargetRange &&
!hasAssignableElements!TargetRange)
Copy link
Contributor

@wilzbach wilzbach Jan 17, 2018

Choose a reason for hiding this comment

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

Only the last two overloads differ, so static if could simply things and provide better error messages here.
I assume you wanted to make it explicit that void is returned? The docs already do so.

Copy link
Member

@schveiguy schveiguy Jan 18, 2018

Choose a reason for hiding this comment

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

Not sure what you are meaning here, but I don't know how you do auto ref correctly without an overload.

Copy link
Contributor

@wilzbach wilzbach Jan 18, 2018

Choose a reason for hiding this comment

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

Ah fair enough :/

{
put(target, source);
}

///
@safe unittest
{
Expand Down
8 changes: 4 additions & 4 deletions std/digest/package.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ version(ExampleDigest)

auto oneMillionRange = repeat!ubyte(cast(ubyte)'a', 1000000);
auto ctx = makeDigest!MD5();
copy(oneMillionRange, &ctx); //Note: You must pass a pointer to copy!
oneMillionRange.copy(ctx);
assert(ctx.finish().toHexString() == "7707D6AE4E027C70EEA2A935C2296F21");
}

Expand Down Expand Up @@ -433,10 +433,10 @@ DigestType!Hash digest(Hash, Range)(auto ref Range range)
if (!isArray!Range
&& isDigestibleRange!Range)
{
import std.algorithm.mutation : copy;
import std.range.primitives : put;
Hash hash;
hash.start();
copy(range, &hash);
put(hash, range);
Copy link
Member

@schveiguy schveiguy Jan 17, 2018

Choose a reason for hiding this comment

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

Can revert, and also switch to UFCS if you want.

Copy link
Contributor Author

@JackStouffer JackStouffer Jan 17, 2018

Choose a reason for hiding this comment

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

Since this one isn't a public example, and std.range is more likely to have already been imported, I'll leave this

Copy link
Member

@schveiguy schveiguy Jan 18, 2018

Choose a reason for hiding this comment

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

OK, I didn't notice that it wasn't an example.

return hash.finish();
}

Expand Down Expand Up @@ -634,7 +634,7 @@ interface Digest

auto oneMillionRange = repeat!ubyte(cast(ubyte)'a', 1000000);
auto ctx = new MD5Digest();
copy(oneMillionRange, ctx);
oneMillionRange.copy(ctx);
assert(ctx.finish().toHexString() == "7707D6AE4E027C70EEA2A935C2296F21");
}

Expand Down
10 changes: 5 additions & 5 deletions std/stdio.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ module std.stdio;

import core.stdc.stddef; // wchar_t
public import core.stdc.stdio;
import std.algorithm.mutation; // copy
import std.meta; // allSatisfy
import std.range.primitives; // ElementEncodingType, empty, front,
// isBidirectionalRange, isInputRange, put
Expand Down Expand Up @@ -3143,7 +3142,7 @@ void main()

@system unittest
{
import std.algorithm.mutation : reverse;
import std.algorithm.mutation : copy, reverse;
import std.exception : collectException;
static import std.file;
import std.range : only, retro;
Expand Down Expand Up @@ -4572,16 +4571,17 @@ private struct ChunksImpl

/**
Writes an array or range to a file.
Shorthand for $(D data.copy(File(fileName, "wb").lockingBinaryWriter)).
Shorthand for `put(data, File(fileName, "wb").lockingBinaryWriter)`.
Similar to $(REF write, std,file), strings are written as-is,
rather than encoded according to the $(D File)'s $(HTTP
en.cppreference.com/w/c/io#Narrow_and_wide_orientation,
orientation).
*/
void toFile(T)(T data, string fileName)
if (is(typeof(copy(data, stdout.lockingBinaryWriter))))
if (is(typeof({ auto w = stdout.lockingBinaryWriter; put(w, data); })))
{
copy(data, File(fileName, "wb").lockingBinaryWriter);
auto w = File(fileName, "wb").lockingBinaryWriter;
put(w, data);
Copy link
Member

@schveiguy schveiguy Jan 17, 2018

Choose a reason for hiding this comment

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

I'd prefer reverting this, but I'm fine with the new version as well.

Copy link
Contributor Author

@JackStouffer JackStouffer Jan 17, 2018

Choose a reason for hiding this comment

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

I left this one to remove the global std.algorithm.mutation import.

}

@system unittest
Expand Down