Issue 8829 - std.algorithm.find fails to take advantage of SortedRange by RazvanN7 · Pull Request #4907 · dlang/phobos

RazvanN7 · 2016-11-17T10:19:22Z

Added functionality so that find can take advantage of SortedRange's functions when:

A single element is searched in a SortedRange.
A a range is searched in another range and both ranges are SortedRange with the same signature

dlang-bot · 2016-11-17T10:19:24Z

Fix	Bugzilla	Description
✗	5236	[patch] std.format.formattedRead/unformatValue does not support the raw reading of integer types
✗	8829	std.algorithm.find fails to take advantage of SortedRange

andralex

Good going, thanks for this work.

andralex · 2016-11-20T14:33:42Z

std/algorithm/searching.d

+    // Works only for the default find predicate and any SortedRange predicate.
+    // 8829 enhancement
+    import std.range: SortedRange;
+    static if(is(typeof(haystack) : SortedRange!TT, TT) && isDefaultPred)


space after if

You already have the type of haystack: static if (is(InputRange : SortedRange!TT, TT) && isDefaultPred)

andralex · 2016-11-20T14:34:24Z

std/algorithm/searching.d

+    static if(is(typeof(haystack) : SortedRange!TT, TT) && isDefaultPred)
+    {
+        auto partitions = haystack.trisect(needle);
+        if(partitions[1].length == 0)


space after if (throughout please)

andralex · 2016-11-20T14:36:23Z

std/algorithm/searching.d

+    import std.range: SortedRange;
+    static if(is(typeof(haystack) : SortedRange!TT, TT) && isDefaultPred)
+    {
+        auto partitions = haystack.trisect(needle);


trisect does a bit more work than lower/upperBound, could you use one of those instead? Probably lowerBound because you want to find the first occurrence.

andralex · 2016-11-20T14:39:06Z

std/algorithm/searching.d

+    // When it is found O(walklength(needle)) steps are performed.
+    // 8829 enhancement
+    import std.range;
+    static if(is(typeof(haystack) == typeof(needle))


is(R1 == R2)

andralex · 2016-11-20T14:39:36Z

std/algorithm/searching.d

+    // 8829 enhancement
+    import std.range;
+    static if(is(typeof(haystack) == typeof(needle))
+            && is(typeof(haystack) : SortedRange!TT, TT)


is(R1 : SortedRange!TT, TT)

andralex · 2016-11-20T14:41:46Z

std/algorithm/searching.d

+        auto needleFirstElem = needle[0];
+        auto partitions      = haystack.trisect(needleFirstElem);
+        auto firstElemLen    = partitions[1].length;
+        int count            = 0;


andralex · 2016-11-20T14:43:24Z

std/algorithm/searching.d

+        auto firstElemLen    = partitions[1].length;
+        int count            = 0;
+
+        if(firstElemLen == 0)


Replace most of the code below with a call to mismatch: https://dlang.org/library/std/algorithm/comparison/mismatch.html

I made all the changes. Hope it's ok now

andralex

OK, I think these are the right algorithms to use. Please make a pass for the potential bugs. Thx!

andralex · 2016-12-06T05:26:41Z

std/algorithm/searching.d

+    static if (is(InputRange : SortedRange!TT, TT) && isDefaultPred)
+    {
+        auto lb = haystack.lowerBound(needle);
+        if (lb.length == 0 || lb.length == haystack.length || haystack[lb.length] != needle)


Why do you have lb.length == 0 there? It means there is no element less than the needle in the haystack, so the haystack may actually start with the needle. No?

Sorry, that case has eluded me. The condition was for the case when the needle has a value which is smaller then the smallest value in the range. I added an additional condition which rules out the case you mentioned (lb.length == 0 && haystack[0] != needle).

andralex · 2016-12-06T05:29:15Z

std/algorithm/searching.d

+
+        while (needle.front() == needleFirstElem)
+        {
+            auto elem = needle.front();


Why do you need elem?

I don't. I used it in a previous version and forgot about it.

andralex · 2016-12-06T05:31:07Z

std/algorithm/searching.d

+
+        auto m = mismatch(partitions[2], needle);
+
+        if (m[1] == haystack[$ .. $])


if (m[1].empty)

andralex · 2016-12-06T05:31:13Z

std/algorithm/searching.d

+
+        if (m[1] == haystack[$ .. $])
+            return haystack[partitions[0].length + partitions[1].length - count .. $];
+        if (m[1] != haystack[$ .. $])


if (m[1].empty)

andralex · 2016-12-06T05:31:37Z

std/algorithm/searching.d

+            ++count;
+
+            if (count > firstElemLen)
+                return haystack[$ .. $];


This line is not covered, please add to the unittest

…support the raw reading of integer types

andralex · 2016-12-06T13:33:39Z

std/algorithm/searching.d

+        auto lb = haystack.lowerBound(needle);
+        if ((lb.length == 0 && haystack[0] != needle) || lb.length == haystack.length
+           || haystack[lb.length] != needle)
+            return haystack[$ .. $];


Odd alignment - first, there's one character difference which is definitely a problem (all indentation levels should be a multiple of 4). Second, code is (approximately) at the same level as an unrelated expression continuation, which can't be right.

This expression is convoluted:

(lb.length == 0 && haystack[0] != needle) || lb.length == haystack.length || haystack[lb.length] != needle

Simplified:

(x == 0 && haystack[x] != needle) || x == y || haystack[x] != needle

Reordered:

x == y || (x == 0 && haystack[x] != needle) || haystack[x] != needle

And this exposes the redundancy:

x == y || haystack[x] != needle

In fact the more complicated condition had a bug - you're accessing haystack[0] without protection so you'll have a bounds check failure if the haystack is empty.

@wilzbach can you add to the style checker that we detect all indents that are not a multiple of 4?

Done. Thanks. Indeed, the code is much cleaner now.

andralex · 2016-12-06T13:41:25Z

std/algorithm/searching.d

+
+        if (m[1].empty)
+            return haystack[partitions[0].length + partitions[1].length - count .. $];
+        if (!m[1].empty)


This condition is the complement of the previous one, which if passes will terminate the flow. Remove.

I did it because I had some problems in the past with static ifs (the code which followd my block was declared unreachable), but I solved now. Thanks

nordlow · 2016-12-07T11:05:16Z

Here are some more algorithms that should be specialized: #3534

The algorithms isSorted and {min,max}Element should be the lowest hanging fruits.

schuetzm · 2016-12-09T10:24:08Z

This should have been squashed before pulling.

andralex · 2016-12-09T13:43:27Z

@schuetzm can we do it postmortem? cc @CyberShadow

wilzbach · 2016-12-09T14:01:35Z

@schuetzm can we do it postmortem? cc @CyberShadow

Nope rebasing the git history is no option, but maybe we can at least learn from these events? :)

CyberShadow · 2016-12-09T14:06:02Z

No, we can't.

Don't worry about it. Most of the problems of not squishing commits are when some changes are done in one commit and then undone in another, as that creates noise for blame. Here it seems like most of the commits were fixups on the code added in the first commit, so although a neater history would've been nice, it's not a big deal.

andralex · 2016-12-09T14:22:33Z

OK thank you all!

wilzbach · 2016-12-26T06:43:30Z

FYI this uses string comparisons to check whether a range isSorted:

    static if (is(typeof(pred == "a == b")))
        enum isDefaultPred = pred == "a == b";
    else
        enum isDefaultPred = false;

...

    static if (is(InputRange : SortedRange!TT, TT) && isDefaultPred)

So:

it won't work if the user uses a lambda and I thought we wanted to move away from the string lambda
it won't work if it's applied from another range function that doesn't modify the sortedness attribute (e.g. take)

I opened a bug for using string comparisons and another for propagating sortedness, s.t. it's not forgotten :/

andralex · 2016-12-26T12:38:05Z

@wilzbach cool - the better solution is to use overloads that detect "no lambda has been passed"

RazvanN7 changed the title ~~Issue 8828~~ Issue 8829 Nov 17, 2016

andralex requested changes Nov 20, 2016

View reviewed changes

RazvanN7 changed the title ~~Issue 8829~~ Issue 8829 - std.algorithm.find fails to take advantage of SortedRange Dec 5, 2016

andralex requested changes Dec 6, 2016

View reviewed changes

RazvanN7 added 11 commits December 6, 2016 13:19

Issue 5236 - [patch] std.format.formattedRead/unformatValue does not …

17d13e2

…support the raw reading of integer types

Issue 5236 - [patch] std.format.formattedRead/unformatValue does not …

26f4aec

…support the raw reading of integer types

Issue 5236 - [patch] std.format.formattedRead/unformatValue does not …

7340666

…support the raw reading of integer types

Issue 8829 - std.algorithm.find fails to take advantage of SortedRange

8b29f20

Issue 8829 - std.algorithm.find fails to take advantage of SortedRange

d651985

Issue 8829 - std.algorithm.find fails to take advantage of SortedRange

818d512

Issue 8829 - std.algorithm.find fails to take advantage of SortedRange

752b2ca

Issue 8829 - std.algorithm.find fails to take advantage of SortedRange

e4b8250

added a comment

71ffa25

Fixed some issues

60396a1

Applied review feedback, fixed some bugs + added unit tests for them

2e89652

RazvanN7 force-pushed the Issue_8828 branch from 53c5453 to 2e89652 Compare December 6, 2016 11:52

andralex requested changes Dec 6, 2016

View reviewed changes

Solved a minor bug

ff4b6b9

andralex approved these changes Dec 6, 2016

View reviewed changes

andralex merged commit 98a7c44 into dlang:master Dec 6, 2016

wilzbach mentioned this pull request Jul 24, 2017

Fix Issue 17679 - SortedRange.contains should be deprecated in favor of the generic canFind #5651

Closed

RazvanN7 mentioned this pull request Sep 4, 2017

Fix Issue 9682 - group/filter should return a SortedRange when they are passed one #5712

Closed


		auto m = mismatch(partitions[2], needle);

		if (m[1] == haystack[$ .. $])

Uh oh!

Comments

Conversation

RazvanN7 commented Nov 17, 2016

Uh oh!

dlang-bot commented Nov 17, 2016

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schuetzm commented Dec 9, 2016

Uh oh!

andralex commented Dec 9, 2016

Uh oh!

wilzbach commented Dec 9, 2016

Uh oh!

CyberShadow commented Dec 9, 2016

Uh oh!

andralex commented Dec 9, 2016

Uh oh!

wilzbach commented Dec 26, 2016

Uh oh!

andralex commented Dec 26, 2016

Uh oh!

nordlow commented Dec 7, 2016 •

edited

Loading