Fix #10257 - variadic overload of std.algorithm.searching.countUntil should return which needle was found #5618

wilzbach · 2017-07-16T21:20:53Z

Thanks a lot to @MartinNowak for suggesting this solution on Slack.

CC @RazvanN7

dlang-bot · 2017-07-16T21:20:55Z

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.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#5618"

wilzbach · 2017-07-16T21:22:01Z

std/algorithm/searching.d


    See_Also: $(REF indexOf, std,string)
  +/
-ptrdiff_t countUntil(alias pred = "a == b", R, Rs...)(R haystack, Rs needles)
+auto countUntil(alias pred = "a == b", R, Rs...)(R haystack, Rs needles)
 if (isForwardRange!R
    && Rs.length > 0
    && isForwardRange!(Rs[0]) == isInputRange!(Rs[0])
    && is(typeof(startsWith!pred(haystack, needles[0])))
    && (Rs.length == 1
    || is(typeof(countUntil!pred(haystack, needles[1 .. $])))))


CC @UplinkCoder in case you haven't seen how CTFE is abused at Phobos ;-)

@wilzbach I am painfully aware.

RazvanN7 · 2017-07-17T08:05:11Z

Awesome. Looks good to me.

PetarKirov · 2017-07-16T22:02:25Z

std/algorithm/searching.d

        static if (hasLength!R) //Note: Narrow strings don't have length.
        {
            //We delegate to find because find is very efficient.
            //We store the length of the haystack so we don't have to save it.
            auto len = haystack.length;
            auto r2 = find!pred(haystack, needles[0]);
            if (!r2.empty)
-              return cast(typeof(return)) (len - r2.length);
+              return cast(T) (len - r2.length);


return ptrdiff_t(len - r2.length) and remove the alias T above.

Also use ptrdiff_t(0) and so on on the the few places below, to prevent incorrect return type deduction, due to the use static ifs.

PetarKirov · 2017-07-17T10:26:57Z

std/algorithm/searching.d

@@ -799,15 +802,33 @@ if (isForwardRange!R
                  haystack.popFront();
            }
        }
+        //Because of @@@8804@@@: Avoids both "unreachable code" or "no return statement"


Isn't this simply because there's no return statement in the else branch above?

PetarKirov · 2017-07-17T10:30:34Z

std/algorithm/searching.d

@@ -799,15 +802,33 @@ if (isForwardRange!R
                  haystack.popFront();
            }
        }
+        //Because of @@@8804@@@: Avoids both "unreachable code" or "no return statement"
+        static if (isInfinite!R) assert(0);


!isInfinite!R should be part of the template constraints, IMO.

Why? There are legitimate use cases for using countUntil with an infinite range (I added also a test to ensure that this works)

PetarKirov · 2017-07-17T10:52:41Z

std/algorithm/searching.d

+        {
+            ptrdiff_t steps, needle; // both -1 when nothing was found
+            alias steps this; // compatible with previous ptrdiff_t return type
+            ptrdiff_t opIndex(size_t idx) // faking a tuple


I'm playing with the idea to change the signature to ptrdiff_t opIndex(bool idx). Due to implicit conversion, res[0] and res[1] would still work. Looks like a hack, but since this type models Tuple!(ptrdiff_t, "steps", ptrdiff_t needle) and tuples do not satisfy isRandomAccessRange, we don't need to use size_t either. Using std.typecons.Flag could also work, but I'm failing to find a good name for the template parameter.

Alternatively, if we had DIP66 implemented, we could do:

struct Result { import std.meta : AliasSeq; alias Members = AliasSeq!(ptrdiff_t, ptrdiff_t); Members _members; alias steps = _members[0]; alias needle = _members[1]; alias _members this; // 1) alias steps this; // 2) } unittest { auto r = Result(1, 2); assert(r[0] == 1); // 1) assert(r[1] == 2); // 1) assert(r == 1); // 2) }

But unfortunately you can only choose between 1) or 2). (We also can't use T opCast(T : ptrdiff_t)() { return steps; }, because this requires the use of explicit casting - such as assert(cast(ptrdiff_t)r == 1) - and so it would be a breaking change.)

I'm playing with the idea to change the signature to ptrdiff_t opIndex(bool idx). Due to implicit conversion, res[0] and res[1] would still work.

Yeah that's quite weird. Imho - if we want to improve things - we should look into how we could trigger deprecation warnings if the alias steps this is used, but I'm not sure whether that's possible.

tuples do not satisfy isRandomAccessRange

This emulation isn't even an InputRange.

PetarKirov · 2017-07-17T11:00:27Z

std/algorithm/searching.d

+        result.needle = -1;
+        result.steps = -1;
+        //Because of @@@8804@@@: Avoids both "unreachable code" or "no return statement"
+        static if (isInfinite!R) assert(0);


The assert(0) shouldn't be necessary if you return unconditionally here.

andralex

This looks a bit on the clever side. All we seem to need here is:

struct Result
{
    ptrdiff_t count;
    alias count this;
    size_t match;
}

Then return that guy. We can even generalize that (if it has enough pick-up) to a ExtraData!(ptrdiff_t, size_t) in which the first member is the supertype and the second is "extra data".

Regardless, this is liable to break code - what with people using auto and all. How about just adding a new function countUntilWhich that takes a ref size_t as its second argument?

andralex · 2017-07-17T22:05:39Z

std/algorithm/searching.d

@@ -749,36 +749,38 @@ if (isInputRange!R && !isInfinite!R)
    $(D startsWith!pred(haystack, needles)) is $(D true). If
    $(D startsWith!pred(haystack, needles)) is not $(D true) for any element in
    $(D haystack), then $(D -1) is returned.
+    If more than one needle is provided, `countUntil` will wrap the result
+    in a tuple similar to `Tuple!(ptrdiff_t, "steps", ptrdiff_t needle)`


Did you mean to quote "needle"? Also, why is the tuple "similar to" and not exactly?

Ah, got it it's the pseudo-tuple trick below.

andralex · 2017-07-17T22:09:30Z

std/algorithm/searching.d

    static if (needles.length == 1)
    {
+        ptrdiff_t result;


move this to where it's used (quite a bit down it seems)

wilzbach · 2017-07-17T23:32:25Z

Regardless, this is liable to break code - what with people using auto and all.

Yes in theory that's possible, but isn't this unlikely as the resulting type will behave as if it would be a ptrdiff_t?

How about just adding a new function countUntilWhich that takes a ref size_t as its second argument?

I would like to avoid introducing a new symbol for this seldom use-case. Hence, if we can't do the trick above, I think a template flag would be better e.g. countUntil!Yes.needleCount(haystack, needles).
Also note that countUntil(haystack, needlePosition, needles) would be possible as well.

0xEAB · 2024-12-26T05:00:39Z

TODO: Squash and update the commit message to reference GH issue #10257.

…ntil should return which needle was found

wilzbach requested review from andralex, JackStouffer and PetarKirov as code owners July 16, 2017 21:20

dlang-bot added the Severity:Enhancement label Jul 16, 2017

wilzbach commented Jul 16, 2017

View reviewed changes

PetarKirov reviewed Jul 17, 2017

View reviewed changes

andralex reviewed Jul 17, 2017

View reviewed changes

wilzbach added the Merge:Blocked label Jul 17, 2017

wilzbach force-pushed the fix-176344 branch from 0f4a8d6 to 3bdd307 Compare November 17, 2017 16:57

dlang-bot added Review:Needs Work Merge:stalled Merge:Needs Rebase and removed Merge:stalled Merge:Needs Rebase labels Dec 29, 2017

dlang-bot added the Merge:stalled label May 27, 2021

RazvanN7 force-pushed the fix-176344 branch from 3bdd307 to 331a582 Compare October 15, 2021 13:08

dlang-bot added Severity:Bug Fix and removed Review:Needs Work Merge:Needs Rebase Merge:stalled labels Oct 15, 2021

RazvanN7 approved these changes Oct 15, 2021

View reviewed changes

RazvanN7 removed the Merge:Blocked label Oct 15, 2021

dlang-bot added the Merge:stalled label Oct 15, 2021

LightBender added the Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024

0xEAB changed the title ~~Fix Issue 17634 - variadic overload of std.algorithm.searching.countUntil should return which needle was found~~ Fix #10257 - variadic overload of std.algorithm.searching.countUntil should return which needle was found Dec 26, 2024

0xEAB force-pushed the fix-176344 branch 2 times, most recently from 3bdc293 to d257ce3 Compare December 26, 2024 04:48

Fix dlang#10257 - variadic overload of std.algorithm.searching.countU…

e2468b5

…ntil should return which needle was found

0xEAB force-pushed the fix-176344 branch from d257ce3 to e2468b5 Compare December 26, 2024 19:47

0xEAB requested a review from thewilsonator December 27, 2024 02:09

thewilsonator approved these changes Dec 27, 2024

View reviewed changes

thewilsonator merged commit d895504 into dlang:master Dec 27, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #10257 - variadic overload of std.algorithm.searching.countUntil should return which needle was found #5618

Fix #10257 - variadic overload of std.algorithm.searching.countUntil should return which needle was found #5618

wilzbach commented Jul 16, 2017

dlang-bot commented Jul 16, 2017 •

edited

Loading

wilzbach Jul 16, 2017

UplinkCoder Jul 19, 2017

RazvanN7 commented Jul 17, 2017

PetarKirov Jul 16, 2017 •

edited

Loading

PetarKirov Jul 17, 2017

PetarKirov Jul 17, 2017

wilzbach Jul 17, 2017

PetarKirov Jul 17, 2017

wilzbach Jul 17, 2017

PetarKirov Jul 17, 2017

andralex left a comment

andralex Jul 17, 2017

andralex Jul 17, 2017

andralex Jul 17, 2017

wilzbach commented Jul 17, 2017

0xEAB commented Dec 26, 2024

Fix #10257 - variadic overload of std.algorithm.searching.countUntil should return which needle was found #5618

Fix #10257 - variadic overload of std.algorithm.searching.countUntil should return which needle was found #5618

Conversation

wilzbach commented Jul 16, 2017

dlang-bot commented Jul 16, 2017 • edited Loading

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented Jul 17, 2017

PetarKirov Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Jul 17, 2017

0xEAB commented Dec 26, 2024

dlang-bot commented Jul 16, 2017 •

edited

Loading

PetarKirov Jul 16, 2017 •

edited

Loading