Skip to content

[Static if] replace overload constraints with static if (mutation.d)#5149

Merged
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:static_if_mutation
May 12, 2017
Merged

[Static if] replace overload constraints with static if (mutation.d)#5149
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:static_if_mutation

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Feb 18, 2017

Split up of #5145

Copy link
Contributor Author

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

This contains a fix for @WalterBright's favorite example of the remove overloads.

if (areCopyCompatibleArrays!(SourceRange, TargetRange))
if (isInputRange!SourceRange && (
areCopyCompatibleArrays!(SourceRange, TargetRange) ||
isOutputRange!(TargetRange, ElementType!SourceRange)))
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 next overload which is grouped, is also an InputRange, but also an OutputRange

void initializeAll(Range)(Range range)
if (isInputRange!Range && hasLvalueElements!Range && hasAssignableElements!Range)
if (isInputRange!Range && hasLvalueElements!Range && hasAssignableElements!Range ||
is(Range == char[]) || is(Range == wchar[]))
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 next overload is is(Range == char[]) || is(Range == wchar[]) which can be appended here as && has a higher precedence.

(Range range, Offset offset)
if (s != SwapStrategy.stable
&& isBidirectionalRange!Range
if (isBidirectionalRange!Range
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is @WalterBright's favorite example. There is absolutely no need to expose different overloads depending on the SwapStrategy to the user.

void reverse(Range)(Range r)
if (isBidirectionalRange!Range && !isRandomAccessRange!Range
&& hasSwappableElements!Range)
if (isBidirectionalRange!Range && (hasSwappableElements!Range || isRandomAccessRange!Range))
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 next overload is isRandomAccessRange!Range && hasLength!Range which implies bidirectionality of Range as isRandomAccessRange requires the following:

static assert(isBidirectionalRange!R || isForwardRange!R && isInfinite!R);

Hence it either needs to have swappableElements (current) or is RandomAccess (next)

@wilzbach wilzbach force-pushed the static_if_mutation branch from 59ef3cd to 6552a95 Compare May 5, 2017 11:08
@wilzbach wilzbach requested a review from JackStouffer May 5, 2017 11:09
r.popBack();
}
}
else static if (isNarrowString!Range && !is(ElementType!Range == const) && !is(ElementType!Range == immutable))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested this puts the overloads into one function, s.t. documentation & error message is improved

// leftover move
moveAll(src, tgt);
return result;
else static assert(0, "Unknown SwapStrategy.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, change to SwapStrategy.semistable is not supported

Copy link
Contributor

@JackStouffer JackStouffer left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

@wilzbach wilzbach force-pushed the static_if_mutation branch from 6552a95 to 4c9170a Compare May 12, 2017 10:27
@wilzbach
Copy link
Contributor Author

Looks good otherwise

Addressed the nit.

@wilzbach wilzbach force-pushed the static_if_mutation branch from 4c9170a to c6819e2 Compare May 12, 2017 13:04
@MartinNowak
Copy link
Member

This broke the nightly builds @wilzbach, see http://nightlies.dlang.org/dmd-master-2017-05-13/build.html.
Please either fixup or revert this PR.

I'm also not positive this is a good long-term direction in general, hope we can work on improved error messages for template constraints later this year.

@JackStouffer
Copy link
Contributor

@MartinNowak The motivation is documentation simplification and not error messages.

@MartinNowak
Copy link
Member

https://dlang.org/phobos/std_algorithm_mutation.html#copy
I see, but now it no longer mentions the constraints/required concepts at all.
Of course a private and undocumentd areCopyCompatibleArrays constraints is an anti-pattern as well.

@MartinNowak
Copy link
Member

Reduced test-case:

import std.file, std.algorithm;

void main()
{
    string a, b;
    copy(a, b);
}

This actually seems to break "instantiation failure is not an error", for the copy overloadset, because the constraints are too general, now both imported copy functions are callable, but one already errors during semantic (think within the overload resolution).
Please have a look at preserving the existing constraints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants