Issue 8573 - A simpler Phobos function that returns the index of the … by RazvanN7 · Pull Request #4921 · dlang/phobos

RazvanN7 · 2016-11-24T14:08:20Z

…mix or max item

dlang-bot · 2016-11-24T14:08:27Z

Fix	Bugzilla	Description
✗	8573	A simpler Phobos function that returns the index of the mix or max item

ntrel · 2016-11-25T14:30:18Z

getMinPos vs existing minPos is not descriptive. Maybe call it minPosIndex.
The two versions above should share a single implementation.

There doesn't seem to be a version of find that returns an index. It seems that might be a necessary precedent before adding this PR. There is std.string.indexOf, which is string-specific.

ghost · 2016-11-25T22:53:27Z

std/algorithm/searching.d

+
+Returns:
+    The position of the first encounter of the minimum (respectively maximum) in `range`.
+ */


It should return a ptrdiff_t.

ghost · 2016-11-26T03:56:19Z

std/algorithm/searching.d

+    return min_pos;
+}
+
+///Ditto


same as previously, return a ptrdiff_t

ntrel · 2016-11-26T12:07:02Z

I added an alternate idea using std.range.enumerate to the bug issue.

ntrel · 2016-11-28T15:22:20Z

std/algorithm/searching.d

    assert(minPos!("a[0] < b[0]")(b) == [ [2, 4], [4], [4] ]);
 }

+/**


index of the first occurrence...

ntrel · 2016-11-28T15:26:03Z

std/algorithm/searching.d

+           maximum) element.
+    range = The input range to search.
+
+Returns:


I think index is better than position, and we should mention -1 when the range is empty.

ntrel · 2016-11-28T15:27:37Z

std/algorithm/searching.d

+        is(typeof(binaryFun!pred(range.front, range.front))))
+{
+    if (range.empty) return -1;
+


Should be ptrdiff_t

ntrel · 2016-11-28T15:30:29Z

std/algorithm/searching.d

+    auto min = range.front();
+    for (range.popFront(); !range.empty; range.popFront())
+    {
+        ++cur_pos;


front should not have () parentheses - ditto for min initialisation.
It might be better to avoid copying range.front here.

ntrel · 2016-11-28T15:38:57Z

Using mutable min temporary means this won't work when front is const.

ntrel · 2016-11-28T15:48:27Z

Re: my original comment about precedent with find - Andrei added the bootcamp keyword to the Issue previously, so he approves of this.

burner

The given range should be a forward range otherwise the index is useless as there is no more range to index.

burner · 2016-11-25T14:10:08Z

std/algorithm/searching.d

+Returns:
+    The position of the first encounter of the minimum (respectively maximum) in `range`.
+ */
+int getMinPos(alias pred = "a < b", Range)(Range range)


InputRange feels wrong here. You might get the index of the min element but the elements are gone.
ForwardRange IMO is the correct choice here.

InputRange is a struct so it is passed by value. The range is consumed inside the function, but when it returns, there won't be any problems.

InputRange is a struct so it is passed by value. The function does not consume the range from the caller context.

wilzbach · 2016-11-29T01:13:37Z

Do we really need a new function for this?
Since 2.072 we can use this handy syntax

[2, 4, 1].enumerate.maxElement!`a.value`.index; // 1

or even shorter with the array syntax:

[2, 4, 1].enumerate.maxElement!`a[1]`[0]; // 1

Heck we can even use maxPos:

[2, 4, 1].enumerate.maxPos!`a.value`[0].index - 1; // 1

I thought there was a reluctance against adding one-liners to Phobos.

ntrel · 2016-11-29T13:43:55Z

std/algorithm/searching.d

+ptrdiff_t minPosIndex(alias pred = "a < b", Range)(Range range)
+    if (isInputRange!Range && !isInfinite!Range &&
+        is(typeof(binaryFun!pred(range.front, range.front))))
+{


For efficiency in release mode, minElement (using private extremum) assumes the range is not empty:

assert(!r.empty, "r is an empty range");

In that case we can use size_t instead of ptrdiff_t. (Forgot to mention this yesterday).

I think we're good here.

ntrel · 2016-11-29T13:53:02Z

std/algorithm/searching.d


    ptrdiff_t min_pos = 0, cur_pos = 0;
-    auto min = range.front;
+    Unqual!(typeof(range.front)) min = range.front;


Needs a unittest for const front.

which will fail, as min needs to be a Rebindable

@burner: Yes, Rebindable should be made a generic type constructor, not just for reference types. The main change needed for that is to get #4363 in good shape and merged, the other value types should be trivial.

andralex · 2016-12-06T04:57:18Z

If I assign a task to bootcamp it means I put it up for review of the bootcamper, but not necessarily that the resolution of the bug would mean add a new artifact.

In this case people look for the index of extrema all the time and all of our alternatives are roundabout. I'll allow this.

andralex

So let's get this in good shape and merge.

andralex · 2016-12-06T04:57:44Z

std/algorithm/searching.d

+Returns:
+    The index of the first encounter of the minimum (respectively maximum) in `range`. If the
+    range is empty, -1 is returned.
+ */


Return ssize_t instead.

ssize_t isn't a recognized type. If you meant size_t, then this won't work since size_t is an uint and we need to return -1 when the range is empty.

My bad. It seems that it does pass the unit test and it returns -1 even if return type is size_t. I assume it implicitly converts -1 to size_t, right?

Yeah, that's not quite what you want. ssize_t is used in druntime. I guess we can use Signed!size_t.

Ok, got it. Thanks

andralex · 2016-12-06T04:58:34Z

std/algorithm/searching.d

+    The index of the first encounter of the minimum (respectively maximum) in `range`. If the
+    range is empty, -1 is returned.
+ */
+ptrdiff_t minPosIndex(alias pred = "a < b", Range)(Range range)


Require a forward range. Input ranges are odd, e.g. if you copy the front of an input range that may be overwritten by advancing the input range (see byLine).

andralex · 2016-12-06T04:58:59Z

std/algorithm/searching.d

+    range is empty, -1 is returned.
+ */
+ptrdiff_t minPosIndex(alias pred = "a < b", Range)(Range range)
+    if (isInputRange!Range && !isInfinite!Range &&


is(typeof(binaryFun!pred(range.front, range.front))) == bool)

andralex · 2016-12-06T04:59:22Z

std/algorithm/searching.d

+ptrdiff_t minPosIndex(alias pred = "a < b", Range)(Range range)
+    if (isInputRange!Range && !isInfinite!Range &&
+        is(typeof(binaryFun!pred(range.front, range.front))))
+{


I think we're good here.

andralex · 2016-12-06T05:01:11Z

std/algorithm/searching.d

+{
+    if (range.empty) return -1;
+
+    ptrdiff_t min_pos = 0, cur_pos = 0;


Don't do this because not all types are copyable to their unqualified type. Use a second range that trails the first one and is positioned at the extremum found so far.

I added a static if condition which copies only range.front when possible and uses a second range to trail the first range otherwise. Hope this is ok

andralex · 2016-12-06T05:01:46Z

std/algorithm/searching.d

+        is(typeof(binaryFun!pred(range.front, range.front))))
+{
+    if (range.empty) return -1;
+


We don't use underscores in variable names, only in package and module names. Use thisConvention.

andralex · 2016-12-06T05:02:09Z

std/algorithm/searching.d

+    Unqual!(typeof(range.front)) min = range.front;
+    for (range.popFront(); !range.empty; range.popFront())
+    {
+        ++cur_pos;


space after if. @wilzbach we really should catch this automatically :)

We do, but our automatic style checking got disabled this autumn. I am playing catching up since a while now -> #4850

9il · 2016-12-07T09:21:46Z

std/algorithm/searching.d

+Returns:
+    The index of the first encounter of the minimum (respectively maximum) in `range`. If the
+    range is empty, -1 is returned.
+ */


sizediff_t

9il · 2016-12-07T09:22:46Z

std/algorithm/searching.d

+        is(typeof(binaryFun!pred(range.front, range.front)) == bool))
+{
+    if (range.empty) return -1;
+


sizediff_t

9il · 2016-12-07T09:24:05Z

std/algorithm/searching.d

+{
+    if (range.empty) return -1;
+
+    Signed!size_t minPos = 0, curPos = 0;


Why not to store the element instead of range?

The temporary is better here. Just use Unqual!(...) for min declaration

The reason is lazy ranges and lazy algorithms. front should be called only once if possible

That was my initial solution, but @andralex told me that not all types are copyable to their unqualified type.

The common use case is auto amax = range.map!abs.maxIndex;. OK lets use .front

I added a static if which only copies 1 element when the elements are copyable. Is that ok on your behalf?

9il · 2016-12-07T09:24:18Z

std/algorithm/searching.d

+    return minPos;
+}
+
+///Ditto


sizediff_t

9il · 2016-12-07T09:28:37Z

std/algorithm/searching.d

+Returns:
+    The index of the first encounter of the minimum (respectively maximum) in `range`. If the
+    range is empty, -1 is returned.
+ */


minPosIndex -> minIndex

andralex · 2016-12-08T17:03:47Z

Thx! I see you've also address @burner 's and @9il 's reviews, could you guys approve?

JackStouffer · 2016-12-08T18:14:06Z

std/algorithm/searching.d

+sizediff_t minIndex(alias pred = "a < b", Range)(Range range)
+    if (isForwardRange!Range && !isInfinite!Range &&
+        is(typeof(binaryFun!pred(range.front, range.front))))
+{


This runs counter to the behavior of most range functions in Phobos. Normally an empty range is rejected outright with an assert rather than given a special value.

I would change this to assert(!range.empty);

I would say we can return -1 here

I struggled with this for a bit. We have minElement and minCount, which has no reasonable way to report an empty range, so it just explodes. We then have minPos, which elegantly - :) - sidesteps the issue by returning a potentially empty range. This kind of leaves minIndex on its own. There are other functions that must deal with empty ranges, can you guys enumerate a few?

There are other functions that must deal with empty ranges, can you guys enumerate a few?

reduce and fold, #4937 replaces enforce for them with asserts

@9il thx. @JackStouffer other precedents we may draw from?

burner · 2016-11-29T14:35:16Z

std/algorithm/searching.d


    ptrdiff_t min_pos = 0, cur_pos = 0;
-    auto min = range.front;
+    Unqual!(typeof(range.front)) min = range.front;


which will fail, as min needs to be a Rebindable

burner · 2016-12-09T10:56:45Z

std/algorithm/searching.d

+    assert(a.maxIndex == 2);
+    // Range is empty, so return value is -1
+    assert(b.minIndex == -1);
+    // Works for const ranges too


The tests should also test more than the default string lambda.

lambda

another string lambda

Is it ok now?

yes, thank you

andralex · 2016-12-12T13:16:33Z

@JackStouffer I think we're good to go. BTW there's more precedent: http://dlang.org/phobos/std_string.html#.indexOf. Can you please approve today so we move forward. Thanks!

andralex · 2016-12-12T22:13:54Z

@JackStouffer ping

wilzbach

My biggest remark is that we should have a lot more tests! (see comments for details)
It will really help to prevent regressions (and sometimes even fix them ahead-of-time) ;-)

wilzbach · 2016-12-13T06:03:18Z

std/algorithm/searching.d

+    return range.minIndex!((a, b) => binaryFun!pred(b, a));
+}
+
+///


I think you should be to add pure nothrow attributes as well here - this well help to ensure we don't break it in the future.

wilzbach · 2016-12-13T06:11:36Z

std/algorithm/searching.d

+    assert(a.minIndex!"a > b" == 2);
+    // Lambda
+    assert(c.minIndex!((a, b) => a < b) == 3);
+}


Please add additional tests for

@nogc (with static immutable)

different input ranges (have a look in std.range for DummyRange -> e.g. have a look here or here

test with strings

Infinite ranges (static assert)

Immutable ranges

With common element types (classes, structs, tuples)

wilzbach · 2016-12-13T06:12:36Z

std/algorithm/searching.d

+    assert(a.minIndex == 3);
+    // Maximum is 4 and first occurs in position 2
+    assert(a.maxIndex == 2);
+    // Range is empty, so return value is -1


maybe it's just me, but I do prefer splitting this into separate unittest blocks - they then will also generated more nice on dlang.org under the "Examples" section & it also helps the reader to keep track of the arrays.
At the very least it would be nice to group them like here:

int[] a = [2, 3, 4, 1, 2, 4, 1, 1, 2]; // Minimum is 1 and first occurs in position 3 assert(a.minIndex == 3); int b[]; assert(b.minIndex == -1);

wilzbach · 2016-12-13T06:16:47Z

std/algorithm/searching.d

+    }
+    return minPos;
+}
+


Unfortunately ditto isn't that smart - it will just copy over the entire documentation string.
I just saw that's the same style as min/maxPos, but to be honest I am a fan of splitting up the docs, s.t. the description, test and returns nicely fit to the according function. A good example and very related example is minElement:

https://dlang.org/phobos/std_algorithm_searching.html#.minElement

wilzbach · 2016-12-13T06:19:07Z

std/algorithm/searching.d

+        {
+            min = range.front;
+            minPos = curPos;
+        }


Please add a special path for arrays - unfortunately it will be a lot faster.
Take a look at extremum

andralex · 2016-12-13T15:01:43Z

@JackStouffer I'll dismiss the review but not in protest :). Feel free to intervene later, we have time til the next release.

andralex · 2016-12-13T15:02:48Z

oh I see @wilzbach has a review as well - @RazvanN7 could you please take a look

RazvanN7 · 2016-12-14T09:20:21Z

Done @andralex . Thank you

RazvanN7 · 2016-12-14T09:20:50Z

@wilzbach Is it ok now?

wilzbach

Thanks - it looks a lot better now!
I took another pass ;-)

wilzbach · 2016-12-14T15:04:18Z

std/algorithm/searching.d

+    {
+        static if (isForwardRange!DummyType && !isInfinite!DummyType)
+        {
+            DummyType d;


you can also define your custom payload, e.g. you could add sth. like:

d.arr = [5, 3, 7, 2, 1, 4]; assert(d.minIndex = 4); d.arr = []; assert(d.minIndex = -1);

wilzbach · 2016-12-14T15:08:22Z

std/algorithm/searching.d

+    import std.range: cycle;
+    static assert(!__traits(compiles, cycle([1]).minIndex));
+
+    // with all dummy ranges


very minor nitpick: The Phobos style is to use selective imports whenever only a few symbols are needed. For local imports this is nearly always the case ;-)
The motivation is that if the symbols are listed explicitly, it's really easy to see for a reader where the symbols are coming from. We'll hopefully add a check for this soon.
-> import std.internal.test.dummyrange : DummyRange

wilzbach · 2016-12-14T16:08:46Z

std/algorithm/searching.d

+    // with strings
+    assert(["b", "a", "c"].minIndex == 1);
+
+    // infinite range


D style is import std.range : cycle;, the motivation here was to have a unified style. This is even enforced by our CircleCi bot, though I just [realized[(https://github.com//pull/4955) that the style fix doesn't propagate to PR builds.
Btw you can run the style checks locally with make -f posix.mak style

wilzbach · 2016-12-14T16:12:25Z

std/algorithm/searching.d

+    assert(b.minIndex == -1);
+
+    const int[] c = [2, 5, 4, 1, 2, 3];
+    // Works for const ranges too


Huh - you left one instance of maxIndex here.
Btw in general a a user expects that it works with const. So it doesn't make much sense to add it to the public unittest, but to the internal ones

wilzbach · 2016-12-14T16:18:50Z

std/algorithm/searching.d

+    const int[] c = [2, 5, 4, 1, 2, 3];
+    // Works for const ranges too
+    assert(c.maxIndex == 1);
+    // Lambda


How about using a custom type here - could be a lot less confusing and closer to the real world (to flip the predicate one would use max):

struct Dog { int age; } auto dogs = [Dog(10), Dog(5), Dog(15)]; assert(dugs.minIndex!`d1.age < d2.age` == 3);

Note that not everyone is a friend of string lambda, but imho they are awesomely concise and thus well fitted for short example snippets.

wilzbach · 2016-12-14T16:21:09Z

std/algorithm/searching.d

+    {
+        static if (isForwardRange!DummyType && !isInfinite!DummyType)
+        {
+            DummyType d;


see the comment over at minIndex on passing in a custom payload

wilzbach · 2016-12-14T16:22:39Z

std/algorithm/searching.d

+    range = The input range to search.
+
+Returns:
+    The index of the first encounter of the minimum element in `range`. If the


You are inconsistent here, In the last sentence you don't escape range.
FYI function parameters get automatically escaped in ddoc ;-)

wilzbach · 2016-12-14T16:23:08Z

std/algorithm/searching.d

+    range = The input range to search.
+
+Returns:
+    The index of the first encounter of the maximum in `range`. If the


ditto (inconsistency with range)

wilzbach · 2016-12-14T16:27:10Z

std/algorithm/searching.d

+
+Returns:
+    The index of the first encounter of the maximum in `range`. If the
+    range is empty, -1 is returned.


how about:

See_Also: $(REF max, std,algorithm,comparison), $(LREF maxCount), $(LREF maxElement), $(LREF maxPos)

wilzbach · 2016-12-14T16:27:28Z

std/algorithm/searching.d

+
+Returns:
+    The index of the first encounter of the minimum element in `range`. If the
+    range is empty, -1 is returned.


how about

See_Also: $(REF min, std,algorithm,comparison), $(LREF minCount), $(LREF minElement), $(LREF minPos)

RazvanN7 · 2016-12-15T11:26:08Z

@wilzbach Thank you very much for the detailed review. If there is something else that needs changing please tell me.

wilzbach

The examples should should good practices ;-)

wilzbach · 2016-12-15T14:52:11Z

std/algorithm/searching.d

+
+    // Works with more custom types
+    struct Dog { int age; }
+    Dog[] dogs = [Dog(10), Dog(5), Dog(15)];


ehm we want to show the users good practice examples - using maxIndex for the min isn't one -> Please change to sth .like

Dog[] dogs = [Dog(10), Dog(15), Dog(5)]; assert(dogs.maxIndex!"a.age < b.age" == 1);

Solved. Thanks

wilzbach

Thanks a lot for taking the time to address all my comments!

RazvanN7 · 2016-12-15T15:10:55Z

You are welcome, @wilzbach . These are my first PRs as a D contributor, so I really need to get in line with the rigors. Thank you for your review.

wilzbach · 2016-12-15T15:17:29Z

@wilzbach These are my first PRs as a D contributor, so I really need to get in line with the rigors

Looking forward to your next PR!
I think this looks quite nice now, so:

would you be so kind to squash all changes into one commit? (we might support to do that automatically in the near future)
open another PR against the Changelog (as the changelog changes quite often, one quickly runs into merge conflicts. Hence atm the least pain approach is to do so with a separate PR. However, we will move to single changelog files soon).

@JackStouffer with the new GitHub approve/request feature, you are blocking merging:

Merging can be performed automatically once the requested changes are addressed.

andralex · 2016-12-15T15:20:07Z

@JackStouffer seems to be away for a few days now, I'll dismiss his review; there is time for us to address whatever issues are left before releasing.

There seems to be agreement -1 is okay to return

…mix or max item Issue 8573 - A simpler Phobos function that returns the index of the mix or max item added some review fixes fixed an issue with a mutable variable Applied review feedback Renamed functions to minIndex and maxIndex + used sizediff_t for return value type Updated function so that it works optimally even for lazy ranges and algorithms Reverted to having only copyable elements in ranges Added more unittests; implemented an array path; fixed documentation Squashed commits

RazvanN7 · 2016-12-16T10:13:18Z

@wilzbach I squashed the commits and made another PR with the changelog updated accordingly; see #4960.

wilzbach · 2016-12-16T14:41:33Z

Auto-merge toggled on

JackStouffer · 2016-12-20T21:49:17Z

@andralex Yeah, sorry. Was busy moving back to the US :P

I still think that a contract with an assert would be best, however I no longer see it as deal breaking because of the precedent with indexOf.

ghost suggested changes Nov 25, 2016

View reviewed changes

ghost reviewed Nov 26, 2016

View reviewed changes

std/algorithm/searching.d Outdated

return min_pos;

}

///Ditto

Copy link

ghost Nov 26, 2016

Choose a reason for hiding this comment

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

same as previously, return a ptrdiff_t

ntrel requested changes Nov 28, 2016

View reviewed changes

burner requested changes Nov 28, 2016

View reviewed changes

ntrel reviewed Nov 29, 2016

View reviewed changes

9il added @andralex Review:Needs Decision labels Nov 30, 2016

andralex requested changes Dec 6, 2016

View reviewed changes

andralex added Review:Needs Work and removed @andralex Review:Needs Decision labels Dec 6, 2016

RazvanN7 force-pushed the Issue_8573 branch from b2dd64b to d7ce348 Compare December 6, 2016 13:48

9il suggested changes Dec 7, 2016

View reviewed changes

andralex approved these changes Dec 8, 2016

View reviewed changes

9il approved these changes Dec 8, 2016

View reviewed changes

JackStouffer previously requested changes Dec 8, 2016

View reviewed changes

burner requested changes Dec 9, 2016

View reviewed changes

burner approved these changes Dec 12, 2016

View reviewed changes

wilzbach suggested changes Dec 13, 2016

View reviewed changes

RazvanN7 force-pushed the Issue_8573 branch from 856b8f9 to 88a7abf Compare December 14, 2016 09:13

wilzbach mentioned this pull request Dec 14, 2016

[CircleCi] updatebuild script: merge with upstream branch #4955

Merged

wilzbach suggested changes Dec 14, 2016

View reviewed changes

RazvanN7 force-pushed the Issue_8573 branch from e36e2f7 to 1164532 Compare December 15, 2016 14:35

wilzbach suggested changes Dec 15, 2016

View reviewed changes

RazvanN7 force-pushed the Issue_8573 branch from 1164532 to b440df0 Compare December 15, 2016 14:55

wilzbach approved these changes Dec 15, 2016

View reviewed changes

wilzbach added pending changelog and removed Review:Needs Work labels Dec 15, 2016

RazvanN7 force-pushed the Issue_8573 branch from b440df0 to dba20d7 Compare December 16, 2016 09:05

RazvanN7 force-pushed the Issue_8573 branch from dba20d7 to d2c7d37 Compare December 16, 2016 09:12

wilzbach removed the pending changelog label Dec 16, 2016

wilzbach merged commit f3a8401 into dlang:master Dec 16, 2016

wilzbach added pending changelog and removed pending changelog labels Dec 17, 2016

wilzbach mentioned this pull request Dec 21, 2016

Add new min/maxIndex symbols to booktables #4980

Merged

Uh oh!

Conversation

RazvanN7 commented Nov 24, 2016

Uh oh!

dlang-bot commented Nov 24, 2016

Uh oh!

ntrel commented Nov 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntrel commented Nov 26, 2016

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!

ntrel commented Nov 28, 2016

Uh oh!

ntrel commented Nov 28, 2016

Uh oh!

burner 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!

wilzbach commented Nov 29, 2016

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 commented Dec 6, 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!

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!