Provide pred-only overload to std.algorithm.searching.findSkip by wilzbach · Pull Request #5577 · dlang/phobos

wilzbach · 2017-07-08T01:13:26Z

As I pointed out in #5497 (comment), findSkip is very similar to skipOver - in fact before #5497 neither skipOver nor findSkip has a pred-only overload.
My point is that there's no argument to add this overload to skipOver and not to findSkip - on the contrary there's a even a case for only findSkip that @CyberShadow pointed out:

That loop simply consumes the two input ranges for comparison; it does not consume multiple instances of the "prefix". Consider the difference between s.skipOver(' ') and s.skipOver!(c => c == ' ') - intuitively one may think that these should behave the same, but they don't!

findSkip intuitively implies that it loops.

dlang-bot · 2017-07-08T01:13:27Z

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 · 2017-07-08T01:14:17Z

std/algorithm/searching.d

+bool findSkip(alias pred, R1)(ref R1 haystack)
+if (isForwardRange!R1 && is(typeof(unaryFun!pred(haystack.front))))
+{
+    return skipOver!(pred, R1)(haystack);


Of course, if we decide to change the behavior introduced in #5543, we could easily inline #5543 here ...

We certainly do, as seems like that idea has been pre-approved.

CyberShadow · 2017-07-08T01:16:40Z

std/algorithm/searching.d

+bool findSkip(alias pred, R1)(ref R1 haystack)
+if (isForwardRange!R1 && is(typeof(unaryFun!pred(haystack.front))))
+{
+    return skipOver!(pred, R1)(haystack);


We certainly do, as seems like that idea has been pre-approved.

CyberShadow · 2017-07-08T01:16:56Z

std/algorithm/searching.d

+* the first occurrence of $(D needle); otherwise $(D false),
+* leaving $(D haystack) untouched.
+*/
+bool findSkip(alias pred, R1)(ref R1 haystack)


Would be nice if this could return a size_t instead of just a bool.

Sure, I just would like to wait for @andralex's opinion on this matter before investing more work.

wilzbach · 2017-07-09T00:51:10Z

Would be nice if this could return a size_t instead of just a bool.

Ah - it was really trivial, so I added support for returning size_t quickly. I wonder whether skipOver should also return size_t ...

andralex · 2017-07-25T16:51:06Z

std/algorithm/searching.d

+*
+* Returns: The number of times `pred` was truthfully evaluated.
+*/
+size_t findSkip(alias pred, R1)(ref R1 haystack)


predicate must be ifTestable

andralex · 2017-07-25T16:52:52Z

std/algorithm/searching.d

+    {
+        haystack.popFront();
+        c++;
+    }


Why not a loop with initial test? No need to duplicate the test in the prelude.

size_t result; while (!haystack.empty && unaryFun!pred(haystack.front)) { ++result; } return result;

andralex · 2017-07-25T16:55:08Z

I'm OK with the addition.

JackStouffer · 2017-07-27T18:38:26Z

Ping @wilzbach

wilzbach · 2017-11-17T16:47:35Z

@JackStouffer @ZombineDev I finally got around rebasing this PR. It should be ready now (:

andralex

a doc nit

andralex · 2017-11-17T17:02:44Z

std/algorithm/searching.d


+/**
+* Advances the `haystack` as long as `pred` evaluates to `true`.
+* The `haystack` is positioned before position of first falsely evaluation of `pred`.


The haystack is positioned so as pred evaluates to false for haystack.front.

wilzbach commented Jul 8, 2017

View reviewed changes

wilzbach changed the title ~~Provide pred-only overload to std.algorithm.searching~~ Provide pred-only overload to std.algorithm.searching.findSkip Jul 8, 2017

wilzbach force-pushed the algorithm-find-skip branch from 47ba808 to 82dffd5 Compare July 8, 2017 01:15

CyberShadow reviewed Jul 8, 2017

View reviewed changes

wilzbach added this to the 2.076.0 milestone Jul 9, 2017

wilzbach added the @andralex label Jul 9, 2017

wilzbach requested a review from andralex July 9, 2017 00:40

wilzbach force-pushed the algorithm-find-skip branch from 82dffd5 to c1c2df6 Compare July 9, 2017 00:50

andralex requested changes Jul 25, 2017

View reviewed changes

andralex removed the @andralex label Jul 25, 2017

JackStouffer removed this from the 2.076.0 milestone Jul 27, 2017

JackStouffer added Review:Needs Work Severity:Enhancement labels Jul 27, 2017

JackStouffer self-assigned this Jul 27, 2017

Provide pred-only overload to std.algorithm.searching.findSkip

42236b7

wilzbach force-pushed the algorithm-find-skip branch from c1c2df6 to 42236b7 Compare November 17, 2017 16:46

wilzbach requested review from JackStouffer and PetarKirov as code owners November 17, 2017 16:46

wilzbach removed the Review:Needs Work label Nov 17, 2017

andralex approved these changes Nov 17, 2017

View reviewed changes

JackStouffer approved these changes Nov 17, 2017

View reviewed changes

MetaLang added 2 commits November 19, 2017 00:31

Change comment as requested by Andrei

dd70c99

More precisely define what this overload returns

67e8536

MetaLang approved these changes Nov 19, 2017

View reviewed changes

JackStouffer added the Merge:auto-merge label Nov 19, 2017

dlang-bot merged commit 7a2732e into dlang:master Nov 19, 2017

wilzbach deleted the algorithm-find-skip branch December 11, 2017 02:15

Uh oh!

Conversation

wilzbach commented Jul 8, 2017

Uh oh!

dlang-bot commented Jul 8, 2017

Bugzilla references

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!

wilzbach commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Jul 25, 2017

Uh oh!

JackStouffer commented Jul 27, 2017

Uh oh!

wilzbach commented Nov 17, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments