Skip to content

[Static if] Replace overload constraints with static if (comparison.d)#5147

Merged
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:static_if_std_algorithm_comparison
Nov 20, 2017
Merged

[Static if] Replace overload constraints with static if (comparison.d)#5147
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:static_if_std_algorithm_comparison

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Feb 18, 2017

Split up of #5145

@wilzbach wilzbach force-pushed the static_if_std_algorithm_comparison branch from c32691f to 31da914 Compare February 18, 2017 02:50
*/
int cmp(alias pred = "a < b", R1, R2)(R1 r1, R2 r2)
if (isInputRange!R1 && isInputRange!R2 && !(isSomeString!R1 && isSomeString!R2))
if (isInputRange!R1 && isInputRange!R2 || isSomeString!R1 && isSomeString!R2)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (isInputRange!R1 && isInputRange!R2 && isSomeChar!(ElementType!R1) && isSomeChar!(ElementType!R2))

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you have to be very careful with isSomeString in template constraints for range-based functions, because enums pass isSomeString but are not ranges.

if (isInputRange!R1 && isInputRange!R2 || isSomeString!R1 && isSomeString!R2)
{
for (;; r1.popFront(), r2.popFront())
static if (isInputRange!R1 && isInputRange!R2)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will be true for all inputs, the is some string overload needs to come first

@wilzbach
Copy link
Contributor Author

if (isInputRange!R1 && isInputRange!R2 && isSomeChar!(ElementType!R1) && isSomeChar!(ElementType!R2))

@JackStouffer :

  • I think you meant ... sInputRange!R2 || isSomeChar... ?
  • FWIW I just copied the existing overloads to one function. If we change them, wouldn't this be a breaking change?

edit: as CircleCi and the auto-tester showed, it really would be a breaking change...
-> reverted your suggestions

@JackStouffer
Copy link
Contributor

My point was that the isSomeString checks are redundant because all strings are going to be character ranges.

The auto tester errors look like the non-string inputs weren't being sent to the input range code path but instead being sent to the string code path.

@wilzbach wilzbach force-pushed the static_if_std_algorithm_comparison branch from efea298 to 2f7a492 Compare July 9, 2017 17:01
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@wilzbach wilzbach force-pushed the static_if_std_algorithm_comparison branch from 2f7a492 to 8097b12 Compare July 9, 2017 17:05
@wilzbach
Copy link
Contributor Author

wilzbach commented Jul 9, 2017

My point was that the isSomeString checks are redundant because all strings are going to be character ranges.

Finally came around rebasing this one :)

@wilzbach wilzbach requested a review from JackStouffer July 9, 2017 17:06
@wilzbach wilzbach added the Review:Trivial typos, formatting, comments label Jul 18, 2017
@wilzbach wilzbach force-pushed the static_if_std_algorithm_comparison branch from 8097b12 to 171e81d Compare November 20, 2017 12:51
@wilzbach
Copy link
Contributor Author

Finally came around rebasing this one :)

... and another rebase. This PR just combines the overloads of cmp into one. It just moves the regarding bodies inside.

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. Go ahead and merge when the CI failures are fixed

for (;; r1.popFront(), r2.popFront())
{
if (r1.empty) return -cast(int)!r2.empty;
if (r2.empty) return !r1.empty;
Copy link
Member

@andralex andralex Nov 20, 2017

Choose a reason for hiding this comment

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

I just noticed this is inefficient because at this point it is known that r1 is not empty. Normally you'd need to make a separate PR for that, but let's not let protocol getting in the way of good work. And how about you eliminate the cast - it's a nice conversion:

if (r1.empty) return -int(r2.empty);
if (r2.empty) return 1;

This will fold two calls to r2.empty into one and perhaps generates better code:

immutable e1 = r1.empty, e2 = r2.empty;
if (e1) return -int(e2);
if (e2) return 1;

@dlang-bot dlang-bot merged commit d15447f into dlang:master Nov 20, 2017
@wilzbach wilzbach deleted the static_if_std_algorithm_comparison branch December 11, 2017 02:15
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.

5 participants