Fix Issue 6787 - Implement std.algorithm.lazySort #1886

Poita · 2014-01-26T19:13:15Z

https://d.puremagic.com/issues/show_bug.cgi?id=6787

DmitryOlshansky · 2014-01-27T11:20:25Z

LGTM

JakobOvrum · 2014-01-28T00:17:58Z

std/algorithm.d

+            _heap.removeFront();
+        }
+    }
+    return Result(r);


You could save some code by passing heapify!more(store) here instead of using a constructor... but I guess it's partially a stylistic thing, as sometimes a constructor is strictly required for its special "cooking" privileges.

quickfur · 2014-01-28T02:39:20Z

Should the struct be a private module-level struct instead of a voldemort? Last I heard, we have problems with voldemorts when alias parameters are involved.

monarchdodra · 2014-01-28T11:16:28Z

std/algorithm.d

+    if (isRandomAccessRange!Range &&
+        hasAssignableElements!Range &&
+        !isInfinite!Range &&
+        is(typeof(binaryFun!less(r.front, r.front)) : bool))


I think the is(typeof(...) : bool) is a non-pattern. The only thing it buys us, is turning down perfectly valid predicates. I don't have an example a sorting operator, but for a "test" operator, canFind returns an int...

monarchdodra · 2014-01-28T11:19:04Z

Should the struct be a private module-level struct instead of a voldemort? Last I heard, we have problems with voldemorts when alias parameters are involved.

Yes, if the voldemort internal struct is declared static, then it will fail to compile if the passed predicate requires a context. Any "ResultType" that depends on a predicate (or more generally, and alias parameter), must not be voldemort. Also (I seem to remember), that if we declare it as non-static, there may be some odd behavior if you do things like assignments, or save. Long story short: global private is better suited for several reasons.

- Using documented unit test - De-Voldemortification - Removed bool check - Fixed import order - Duplicate element unit test

JakobOvrum · 2014-01-29T01:39:28Z

Long story short: global private is better suited for several reasons.

I think it's terrible that we have to make these fundamental style decisions based on DMD bugs...

braddr · 2014-01-29T02:18:18Z

Well, strictly speaking you don't. The code is there for anyone sufficiently motivated to address the issue(s). There's a number of helpful contributors you could talk to about the specific issues and they might be willing to help with the fix or to even do it themselves. Taking the work around approach is just one option, not the only one, and not even the best one.

monarchdodra · 2014-01-29T10:43:12Z

Well, strictly speaking you don't. The code is there for anyone sufficiently motivated to address the issue(s).

Yeah, but fundamentally, I think there is a design issue in there too.

That said, a workaround (I just thought of is):
The main issue is that the Result needs context to the fun encompassing function for context to "get" pred. however, if you actually make Result a template struct that depends on pred, then you can still make it static. In this case, instead of needing context to fun to "get" pred, it has context to pred directly.

auto fun(alias pred)()
{
    static struct Result(alias pred) //Look! I'm static!
    {
        bool foo()
        {
            return pred(5); //No problem!
        }
    }
    return Result!pred();
}

void main()
{
    int k;
    auto s = fun!(a=>a==k)(); //This pred needs context.
}

I think this creates 0 overhead too. I just tested this with map, and it seems to not break anything: map was one of the first Voldemort types to be subject to these bugs.

monarchdodra · 2014-01-29T18:10:20Z

One last thing: For what it's worth, as an end user, I personally dislike working with voldemort return types. When working with them, it seems like you always end up having to deal with Result!(Result!Result, Result) types... which is quite difficult to deal with when you have a long chain and a compile error.

quickfur · 2014-01-29T18:58:55Z

On Wed, Jan 29, 2014 at 10:10:27AM -0800, monarch dodra wrote:

One last thing: For what it's worth, as an end user, I personally
dislike working with voldemort return types. When working with them,
it seems like you always end up having to deal with
Result!(Result!Result, Result) types... which is quite difficult to
deal with when you have a long chain and a compile error.
[...]

This isn't so much a flaw of voldemort types, as a flaw in compiler
error messages. Voldemorts should be qualified with the function name or
name of the enclosing scope instead of just an ambiguous identifier.
Things are much clearer when the error refers to:

chain.Result!(zip.Result!(map!"f(x)".Result), join.Result!R)

than to:

Result!(Result!Result, Result!R)

T

Curiosity kills the cat. Moral: don't be the cat.

ghost · 2014-01-29T19:00:40Z

It's not the only reason why Voldemort's are problematic, another reason is that it's difficult to use them as a type for a field in an aggregate.

Poita · 2014-01-29T19:21:17Z

I think this is getting off topic.

Are there any other issues with this pull that people would like addressing?

monarchdodra · 2014-01-29T19:30:13Z

Are there any other issues with this pull that people would like addressing?

Is the fact that lazySort does a heap sort an "implementation detail", or something that can be counted on? Also, I think it should better document that this range is "destructive" of the original range: Eg: it doesn't just iterate in ascending order, it does a gradual sort.

if we want to document that lazySort does a heap sort, then we should suggest running a reverse afterwards to "finish" sorting the original range.

Poita · 2014-01-29T19:43:26Z

I don't think it should be documented to do a heap sort. That would lock us into a specific implementation that we may regret later.

Edit: ~~I will improve the documentation to make it clear that the range is modified, but I will not specify how it is modified since that could again lock us into a specific implementation.~~

Actually, I think it is pretty well documented as is. The second sentence of the documentation is "$(D r) is modified by the range in-place." Is there a different phrasing you'd rather it used?

monarchdodra · 2014-01-29T20:54:13Z

It's not the only reason why Voldemort's are problematic, another reason is that it's difficult to use them as a type for a field in an aggregate.

That's a different issue (IMO), since most of the time, the return type is deliberately kept unspecified.

andralex · 2014-01-30T04:49:28Z

This is a thin wrapper over the heap. Necessary?

DmitryOlshansky · 2014-01-30T19:36:45Z

@andralex

This is a thin wrapper over the heap. Necessary?

Now that I think of it - a range over heap (as container) would make this stuff obsolete.
How about adding opSlice for BinaryHeap and an example to address the enhancement in bugzilla ?

monarchdodra · 2014-01-30T19:56:32Z

Now that I think of it - a range over heap (as container) would make this stuff obsolete.

I think it'd make even more sense to simply give heap a range interface. It is already a wrapper over something else (not an actual container itself), so I see no problem doing that. It would have more primitives than your basic range, but that shouldn't keep it from being a range itself (I think).

That said, I'm not completely familiar with Heap, so I could be wrong.

Poita · 2014-01-30T22:12:21Z

That makes sense. By the looks of things, we'd just need to add alias popFront = removeFront; and we'd have a range interface for BinaryHeap.

@andralex would you be happy with that?

JakobOvrum · 2014-01-31T04:10:56Z

That makes sense. By the looks of things, we'd just need to add alias popFront = removeFront; and we'd have a range interface for BinaryHeap.

Even if the range must be destructive, we should probably stick to opSlice, for least surprise.

monarchdodra · 2014-01-31T07:37:43Z

Even if the range must be destructive, we should probably stick to opSlice, for least surprise.

Keep in mind Head is not forward, and does not provide save. I think providing opSlice would create more surprise, since it implies a save. Yet if you pop the sliced range, the underlying Heap will have been modified! (And possibly corrupted to boot... it will have inconsistent length).

I'm not even sure an opSlice could work?

JakobOvrum · 2014-02-01T06:52:46Z

Keep in mind Head is not forward, and does not provide save. I think providing opSlice would create more surprise, since it implies a save. Yet if you pop the sliced range, the underlying Heap will have been modified! (And possibly corrupted to boot... it will have inconsistent length).

Yeah, sorry; I should have familiarized myself with BinaryHeap before giving an opinion.

monarchdodra · 2014-02-13T10:58:53Z

@andralex would you be happy with that?

I'd be happy with that.

DmitryOlshansky · 2014-02-25T21:41:38Z

I concur with @monarchdodra on this one, let's make BinaryHeap a range and close the bugzilla of lazySort.

Poita · 2014-03-01T15:51:08Z

I'm not so sure about this. BinaryHeap is meant to be a container, not a range. It feels wrong to be able to destructively iterate a container.

Perhaps this could be a more general facility to consume any range?

struct Consume(Container)
{
    Container _container;
    @property auto ref front() { return _container.front; }
    @property bool empty() { return _container.empty; }
    void popFront() { _container.removeFront(); }
    // ... could add back, indexing, length etc. not saving, not slicing
}
auto consume(Container)(Container c) { return Consume!Container(c); }

This way, you can consume a BinaryHeap to lazily iterate a container destructively. You could also consume and iterate any other container (as removeFront is an axiom of containers).

Thoughts?

DmitryOlshansky · 2014-03-01T21:13:11Z

Perhaps this could be a more general facility to consume any range?

You mean container. It is interesting. I assume with combination with things like take it could be very versatile. I can where front/back goes but there is also removeAny any ideas on extending it otherwise then potentially bidirectional range?

monarchdodra · 2014-03-09T09:01:05Z

I'm not so sure about this. BinaryHeap is meant to be a container, not a range.

According to doc, it's not a container either, it's a mere wrapper. It can wrap either a range, or a container, in which case, it also forwards the container primitives.

andralex · 2014-03-09T19:29:12Z

I think making BinaryHeap a forward range would be awesome.

monarchdodra · 2014-03-09T20:03:54Z

I think making BinaryHeap a forward range would be awesome.

AFAIK, InputRange, but not ForwardRange.

DmitryOlshansky · 2014-03-09T20:04:48Z

I think making BinaryHeap a forward range would be awesome.

Perfect, @Poita - fire at will :)

andralex · 2014-03-09T20:08:04Z

Why wouldn't save work? Oh yes you're right, every step ruins the range for the others. I guess we could make it work by saving the most advanced position etc. but that's onerous.

Poita · 2014-03-09T22:44:28Z

I'll close this pull and make a new one since it's a completely separate approach to the problem.

See discussion here: dlang#1886

Poita · 2014-03-09T23:00:00Z

#1989

Fixes Issue 12358 See discussion here: dlang#1886

Fix Issue 6787 - Implement std.algorithm.lazySort

623e03f

https://d.puremagic.com/issues/show_bug.cgi?id=6787

JakobOvrum reviewed Jan 28, 2014
View reviewed changes

monarchdodra reviewed Jan 28, 2014
View reviewed changes

Addressed review concerns:

a74541d

- Using documented unit test - De-Voldemortification - Removed bool check - Fixed import order - Duplicate element unit test

Poita closed this Mar 9, 2014

Poita added a commit to Poita/phobos that referenced this pull request Mar 9, 2014

Range interface for std.container.BinaryHeap

6ea5cce

See discussion here: dlang#1886

Poita mentioned this pull request Mar 9, 2014

Range interface for std.container.BinaryHeap #1989

Merged

Poita added a commit to Poita/phobos that referenced this pull request Mar 15, 2014

Range interface for std.container.BinaryHeap

83d603d

Fixes Issue 12358 See discussion here: dlang#1886

Uh oh!

Fix Issue 6787 - Implement std.algorithm.lazySort #1886

Fix Issue 6787 - Implement std.algorithm.lazySort #1886

Uh oh!

Conversation

Poita commented Jan 26, 2014

Uh oh!

DmitryOlshansky commented Jan 27, 2014

Uh oh!

JakobOvrum Jan 28, 2014

Choose a reason for hiding this comment

Uh oh!

quickfur commented Jan 28, 2014

Uh oh!

monarchdodra Jan 28, 2014

Choose a reason for hiding this comment

Uh oh!

monarchdodra commented Jan 28, 2014

Uh oh!

JakobOvrum commented Jan 29, 2014

Uh oh!

braddr commented Jan 29, 2014

Uh oh!

monarchdodra commented Jan 29, 2014

Uh oh!

monarchdodra commented Jan 29, 2014

Uh oh!

quickfur commented Jan 29, 2014

Uh oh!

ghost commented Jan 29, 2014

Uh oh!

Poita commented Jan 29, 2014

Uh oh!

monarchdodra commented Jan 29, 2014

Uh oh!

Poita commented Jan 29, 2014

Uh oh!

monarchdodra commented Jan 29, 2014

Uh oh!

andralex commented Jan 30, 2014

Uh oh!

DmitryOlshansky commented Jan 30, 2014

Uh oh!

monarchdodra commented Jan 30, 2014

Uh oh!

Poita commented Jan 30, 2014

Uh oh!

JakobOvrum commented Jan 31, 2014

Uh oh!

monarchdodra commented Jan 31, 2014

Uh oh!

JakobOvrum commented Feb 1, 2014

Uh oh!

monarchdodra commented Feb 13, 2014

Uh oh!

DmitryOlshansky commented Feb 25, 2014

Uh oh!

Poita commented Mar 1, 2014

Uh oh!

DmitryOlshansky commented Mar 1, 2014

Uh oh!

monarchdodra commented Mar 9, 2014

Uh oh!

andralex commented Mar 9, 2014

Uh oh!

monarchdodra commented Mar 9, 2014

Uh oh!

DmitryOlshansky commented Mar 9, 2014

Uh oh!

andralex commented Mar 9, 2014

Uh oh!

Poita commented Mar 9, 2014

Uh oh!

Poita commented Mar 9, 2014

Uh oh!

Uh oh!