Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 9 additions & 9 deletions std/algorithm/mutation.d
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ Params:
*/
void move(T)(ref T source, ref T target)
{
moveImpl(source, target);
moveImpl(target, source);
Copy link
Contributor

@nordlow nordlow Jul 11, 2021

Choose a reason for hiding this comment

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

Why the reversion of parameter order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably should have made a comment in the source code as well

}

/// For non-struct types, `move` just performs `target = source`:
Expand Down Expand Up @@ -1244,7 +1244,7 @@ pure nothrow @safe @nogc unittest
static assert(is(typeof({ S s; move(s, s); }) == T));
}

private void moveImpl(T)(ref T source, ref T target)
private void moveImpl(T)(ref scope T target, ref return scope T source)
{
import std.traits : hasElaborateDestructor;

Expand All @@ -1257,10 +1257,10 @@ private void moveImpl(T)(ref T source, ref T target)
static if (hasElaborateDestructor!T) target.__xdtor();
}
// move and emplace source into target
moveEmplaceImpl(source, target);
moveEmplaceImpl(target, source);
}

private T moveImpl(T)(ref T source)
private T moveImpl(T)(ref return scope T source)
{
// Properly infer safety from moveEmplaceImpl as the implementation below
// might void-initialize pointers in result and hence needs to be @trusted
Expand All @@ -1269,10 +1269,10 @@ private T moveImpl(T)(ref T source)
return trustedMoveImpl(source);
}

private T trustedMoveImpl(T)(ref T source) @trusted
private T trustedMoveImpl(T)(ref return scope T source) @trusted
{
T result = void;
moveEmplaceImpl(source, result);
moveEmplaceImpl(result, source);
return result;
}

Expand Down Expand Up @@ -1415,7 +1415,7 @@ private T trustedMoveImpl(T)(ref T source) @trusted
move(x, x);
}

private void moveEmplaceImpl(T)(ref T source, ref T target)
private void moveEmplaceImpl(T)(ref scope T target, ref return scope T source)
{
import core.stdc.string : memcpy, memset;
import std.traits : hasAliasing, hasElaborateAssign,
Expand Down Expand Up @@ -1486,7 +1486,7 @@ private void moveEmplaceImpl(T)(ref T source, ref T target)
*/
void moveEmplace(T)(ref T source, ref T target) pure @system
{
moveEmplaceImpl(source, target);
moveEmplaceImpl(target, source);
}

///
Expand Down Expand Up @@ -2388,7 +2388,7 @@ Range remove(alias pred, SwapStrategy s = SwapStrategy.stable, Range)(Range rang
@nogc @safe unittest
{
// @nogc test
int[10] arr = [0,1,2,3,4,5,6,7,8,9];
static int[] arr = [0,1,2,3,4,5,6,7,8,9];
alias pred = e => e < 5;

auto r = arr[].remove!(SwapStrategy.unstable)(0);
Expand Down
2 changes: 1 addition & 1 deletion std/array.d
Original file line number Diff line number Diff line change
Expand Up @@ -1953,7 +1953,7 @@ private enum bool hasCheapIteration(R) = isArray!R;
See_Also:
For a lazy version, see $(REF joiner, std,algorithm,iteration)
+/
ElementEncodingType!(ElementType!RoR)[] join(RoR, R)(RoR ror, scope R sep)
ElementEncodingType!(ElementType!RoR)[] join(RoR, R)(RoR ror, R sep)
Copy link
Contributor

Choose a reason for hiding this comment

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

this removes scope I thought this PR was trying to add it, am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Range functions should ideally infer scope based on empty(), popFront(), front(), copy constructors etc.
scope inference is not that good yet however (issue 20674) so sometimes scope is manually applied to help the compiler. With issue 20150 fixed, this scope R sep gives an error:

scope variable sep assigned to non-scope parameter r calling std.array.array!(Once).array

So the long term fix is to ensure scope inference works for both join and array, but that's an enhancement. For now it's important to get the dip1000 bug fix in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this break existing code if the inference fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the accepts-invalid fixes for dip1000 are breaking changes. Luckily dip1000 is still experimental.

Copy link
Contributor

Choose a reason for hiding this comment

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

But these changes are neither specific to DIP1000 nor about actually invalid code. It would be better to first improve the inference s.t. there is no unecessary breakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What code do you think would break without -preview=dip1000?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have aspecific example ATM, will post one if I find a small example.
But we should strive to not break valid code. -dip1000 has existed for quite some time and is certainly used in the wild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dlang/dmd#12010 is going to break such dip1000 code in the wild undoubtedly, I just tested it on my own 37 KLOC dip1000 library and I had to add 3 scope annotations. Do you think we need a second switch preview=dip1000fixed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Neither said nor implied that - some breakage will be unavoidable. I'm just concerned that these changes introduce a temporary regression that could be avoided by first improving the scope inference.

But the actual fallout might be neglegible given that your projects required minimal changes and buildkite passes as is.

if (isInputRange!RoR &&
isInputRange!(Unqual!(ElementType!RoR)) &&
isInputRange!R &&
Expand Down
2 changes: 1 addition & 1 deletion std/datetime/timezone.d
Original file line number Diff line number Diff line change
Expand Up @@ -1702,7 +1702,7 @@ package:
Params:
isoExtString = A string which represents a time zone in the ISO format.
+/
static immutable(SimpleTimeZone) fromISOExtString(S)(S isoExtString) @safe pure
static immutable(SimpleTimeZone) fromISOExtString(S)(scope S isoExtString) @safe pure
if (isSomeString!S)
{
import std.algorithm.searching : startsWith;
Expand Down
4 changes: 2 additions & 2 deletions std/internal/math/biguintcore.d
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ public:

// If wantSub is false, return x + y, leaving sign unchanged
// If wantSub is true, return abs(x - y), negating sign if x < y
static BigUint addOrSubInt(Tulong)(const BigUint x, Tulong y,
static BigUint addOrSubInt(Tulong)(const scope BigUint x, Tulong y,
bool wantSub, ref bool sign) pure nothrow @safe if (is(Tulong == ulong))
{
BigUint r;
Expand Down Expand Up @@ -1380,7 +1380,7 @@ pure nothrow @safe
}

// Encode BigInt as BigDigit array (sign and 2's complement)
BigDigit[] includeSign(const(BigDigit) [] x, size_t minSize, bool sign)
BigDigit[] includeSign(scope const(BigDigit) [] x, size_t minSize, bool sign)
pure nothrow @safe
{
size_t length = (x.length > minSize) ? x.length : minSize;
Expand Down
12 changes: 6 additions & 6 deletions std/path.d
Original file line number Diff line number Diff line change
Expand Up @@ -558,14 +558,14 @@ if (isRandomAccessRange!R && hasSlicing!R && isSomeChar!(ElementType!R) || isNar
the POSIX requirements for the 'dirname' shell utility)
(with suitable adaptations for Windows paths).
*/
auto dirName(R)(R path)
auto dirName(R)(return scope R path)
if (isRandomAccessRange!R && hasSlicing!R && hasLength!R && isSomeChar!(ElementType!R) && !isSomeString!R)
{
return _dirName(path);
}

/// ditto
auto dirName(C)(C[] path)
auto dirName(C)(return scope C[] path)
if (isSomeChar!C)
{
return _dirName(path);
Expand Down Expand Up @@ -662,7 +662,7 @@ if (isSomeChar!C)
//static assert(dirName("dir/file".byChar).array == "dir");
}

private auto _dirName(R)(R path)
private auto _dirName(R)(return scope R path)
{
static auto result(bool dot, typeof(path[0 .. 1]) p)
{
Expand Down Expand Up @@ -1448,7 +1448,7 @@ private auto _withDefaultExtension(R, C)(R path, C[] ext)
Returns: The assembled path.
*/
immutable(ElementEncodingType!(ElementType!Range))[]
buildPath(Range)(Range segments)
buildPath(Range)(scope Range segments)
if (isInputRange!Range && !isInfinite!Range && isSomeString!(ElementType!Range))
{
if (segments.empty) return null;
Expand Down Expand Up @@ -2747,7 +2747,7 @@ else version (Posix)
See_Also:
$(LREF asAbsolutePath) which does not allocate
*/
string absolutePath(return scope string path, lazy string base = getcwd())
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absoultePath calls chainPath(baseVar, path).array, and scope inference fails for .array currently

string absolutePath(string path, lazy string base = getcwd())
@safe pure
{
import std.array : array;
Expand Down Expand Up @@ -2893,7 +2893,7 @@ if (isConvertibleToString!R)
`Exception` if the specified _base directory is not absolute.
*/
string relativePath(CaseSensitive cs = CaseSensitive.osDefault)
(scope return string path, lazy string base = getcwd())
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relativePath calls asRelativePath which takes input ranges, and scope inference fails there

(string path, lazy string base = getcwd())
{
if (!isAbsolute(path))
return path;
Expand Down
2 changes: 1 addition & 1 deletion std/process.d
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ static:
multi-threaded programs. See e.g.
$(LINK2 https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html#Environment-Access, glibc).
*/
inout(char)[] opIndexAssign(inout char[] value, scope const(char)[] name) @trusted
inout(char)[] opIndexAssign(return inout char[] value, scope const(char)[] name) @trusted
{
version (Posix)
{
Expand Down
4 changes: 2 additions & 2 deletions std/random.d
Original file line number Diff line number Diff line change
Expand Up @@ -3362,8 +3362,8 @@ if (isRandomAccessRange!Range)
// Optionally @nogc std.random.randomCover
// https://issues.dlang.org/show_bug.cgi?id=14001
auto rng = Xorshift(123_456_789);
int[5] sa = [1, 2, 3, 4, 5];
auto r = randomCover(sa[], rng);
static immutable int[] sa = [1, 2, 3, 4, 5];
auto r = randomCover(sa, rng);
assert(!r.empty);
const x = r.front;
r.popFront();
Expand Down
4 changes: 2 additions & 2 deletions std/range/package.d
Original file line number Diff line number Diff line change
Expand Up @@ -7708,7 +7708,7 @@ if (isForwardRange!RangeOfRanges &&
@safe unittest
{
import std.algorithm.comparison : equal;
ulong[1] t0 = [ 123 ];
ulong[] t0 = [ 123 ];

assert(!hasAssignableElements!(typeof(t0[].chunks(1))));
assert(!is(typeof(transposed(t0[].chunks(1)))));
Expand Down Expand Up @@ -10850,7 +10850,7 @@ if (isInputRange!Range && !isInstanceOf!(SortedRange, Range))
into a `SortedRange`, it extracts the original range back out of the `SortedRange`
using $(REF, move, std,algorithm,mutation).
*/
auto release()
auto release() return scope
{
import std.algorithm.mutation : move;
return move(_input);
Expand Down
8 changes: 6 additions & 2 deletions std/string.d
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ private:
string _s;
}

bool testAliasedString(alias func, Args...)(scope string s, scope Args args)
bool testAliasedString(alias func, Args...)(string s, Args args)
{
import std.algorithm.comparison : equal;
auto a = func(TestAliasedString(s), args);
Expand Down Expand Up @@ -2632,8 +2632,12 @@ if (isSomeChar!C)

enum S : string { a = "hello\nworld" }
assert(S.a.splitLines() == ["hello", "world"]);
}

char[S.a.length] sa = S.a[];
@system pure nothrow unittest
{
// dip1000 cannot express an array of scope arrays, so this is not @safe
char[11] sa = "hello\nworld";
assert(sa.splitLines() == ["hello", "world"]);
}

Expand Down