Issue 13594: nextPermutation should work with static arrays too. #2650
Conversation
| bool nextPermutation(alias less="a<b", StaticArray)(ref StaticArray arr) | ||
| if (is(StaticArray == T[n], T, size_t n)) | ||
| { | ||
| auto slice = arr[]; |
There was a problem hiding this comment.
Workaround for arr[] not being an lvalue, which is the real cause of the bug.
| assert(a == [3, 2, 1]); | ||
| assert(!a.nextPermutation()); | ||
| assert(a == [1, 2, 3]); | ||
| } |
There was a problem hiding this comment.
Suggest templating the test function and doing all the same tests as the original unit tests. They are already all there, no reason not to reuse them :)
There was a problem hiding this comment.
Good idea. Not sure how to do it in a way that's still simple enough for beginners, though, since the original test is a ddoc'd unittest.
There was a problem hiding this comment.
What about reusing the unit test at line 13311?
|
Pretty simple, looks good to me. |
|
Again, LGTM. Thanks. |
| assert(nextPermutation(a1)); | ||
| assert(equal(a1, [1, 2, 4, 3])); | ||
| assert(nextPermutation(arr)); | ||
| assert(arr == [1, 3, 4, 2]); |
There was a problem hiding this comment.
Why have you replaced equal? The test would no longer work with any Range.
There was a problem hiding this comment.
It's a nested function inside this unit test. I don't think there's any danger of this issue arising, as the only two arguments to this function are a dynamic array and a static array.
There was a problem hiding this comment.
Yeah, this is code inside a unittest, so this shouldn't be an issue.
The only reason I changed it was because equal apparently doesn't know how to compare a dynamic array with a static one. :-(
|
I'm inclined to say NO. If anything this trend would mean that we have to special case every algorithm for static arrays. Just use slice. |
|
The problem is that slice doesn't work because For arrays, the argument doesn't have to be ref, because arrays are already a reference type. But for general ranges, you have to require ref, since you don't know if the range is a reference type or not. But requiring ref means you can't pass a slice to the function. If you have a good solution to this conundrum I'm all ears. |
That doesn't make sense. Why does it need to be an lvalue? If we can remove that requirement, have we fixed this issue? |
|
We need the argument to be ref for value-type ranges, and non-ref for reference-type ranges, because the function needs to mutate the range. |
|
What's a "value-type" range? |
|
Actually, nevermind. I'll put it this way: If |
|
We need to fix nextPermutation so it works on rvalue array slices. This is what any person would expect, especially one used to C++'s iterators. |
|
@quickfur Ehm I guess just require range to have l-value elements, why would something else be needed? |
Yes, exactly. The range itself does not need to be updated, just its elements. |
|
Submitted alternative fix. |
|
IMHO, we should never make a range-based function accept static arrays. The correct thing to do is require that the static array be sliced, and if that doesn't work with the function for some reason, then we need to look at whether it even makes sense for it to accept a slice (if it does, make that work; if it doesn't, then don't make it work with static arrays). In this case, it sounds like making the function accept array slices makes sense, but regardless, we're just asking for it if we make a range-based function accept a static array. They're not ranges. |
|
Yeah, in retrospect that was a bad move. But the alternative fix is simply to allow array slices, which allows the user to pass a slice to a static array where necessary, so that should suffice. |
I apologize also for letting it go so far and asking for updates to unit tests etc without realizing what this was actually for. |
|
No problem, Dmitry caught it and stopped it in time. :-) |
Fixes: https://issues.dlang.org/show_bug.cgi?id=13594