Fix Issue 9616 - SortedRange should support all range kinds #1184
Conversation
|
I'm getting a weird error that may be a regression: Error: expression false ? binarySearch : linear is not a valid template value argument Obviously that's a constant expression... |
|
Can't seem to get the tester to look at this... |
|
I know this was written before the introduction of either feature, but since this already touches the indentation of examples and an alias declaration, we might as well update it to move the examples to unittests and update the alias to use the new syntax. |
|
@JakobOvrum fixed, thanks |
| @@ -8310,7 +8290,7 @@ if (isRandomAccessRange!Range && hasLength!Range) | |||
| } | |||
|
|
|||
| /// Ditto | |||
| @property auto front() | |||
| @property auto ref front() | |||
There was a problem hiding this comment.
If this returns by ref, it means you open SortedRange for mutation, and I think sorted ranges are designed to not allow mutation (AFAIK). I'm not sure this is a correct change.
In any case, if you decide to do this anyways, please also make opIndex (line 8321) also return by auto ref.
There was a problem hiding this comment.
Yah, I think auto ref is fine here. Will make opIndex auto ref as well.
There was a problem hiding this comment.
@monarchdodra The case of searching for something in a SortedRange and then changing it wouldn't work if they aren't open for mutation.. I think this should be allowed. How else would I ever actually find & change something? Remember that you can do changes that don't modify the order, e.g. when you have a Range on an array of class references and the opCmp only uses a few members to do the sorting.
Exactly the thing that is being fixed here is biting me right now.
|
I found some nitpicks, but no real issues. Has anybody else reviewed this? @andralex : Give it a rebase and a some fixes, I think it's almost good to go. |
|
rox |
|
@monarchdodra: LGTM. It will need a rebase though. |
Crap. I missed the window :/ |
|
Pinging @andralex : Sorry I didn't merge it. Now it needs a rebase again. |
|
rebased |
|
uhm, rebased now :) |
The win_32 test suite failed, and it appears to NOT be a fluke: https://d.puremagic.com/test-results/pull.ghtml?projectid=1&runid=928297 |
|
@monarchdodra: thanks. Could please someone with win32 access patch with this request and see what's going on? |
|
On this line (8931): auto r = assumeSorted(f.byLine());
edit: Wait, maybe |
Probably: He used For what its worth, the testers seem to show that win_32 has always been failing, so it wouldn't be a regression. That said, why isn't the win__64_ build failing? Is the case not covered, or is there a difference in behavior? Reading the code, the win_32 failure seems legit, so win_64 not failing smells fishy. |
|
win32 uses Digital Mars' read() function, while win64 uses Microsoft's. These have different behaviors when CRLF's are involved. I suggest looking at the code with an eye towards what happens with CRLF's. |
| // write a sorted range of lines to the file | ||
| f.write("abc\ndef\nghi\njkl"); | ||
| f.close(); | ||
| f.open(name); |
There was a problem hiding this comment.
the bug is here: you need to open it as non-binary: f.open(name, "r");
|
@andralex : Can you fix the failing test please? I put a comment where it needs to be fixed and how. It's the only thing blocking this pull. @AndrejMitrovic : The "nothrow sort" issue is in |
|
Is this pull still passing even after a year since last commit? Anyway feel free to review & pull this, I don't have objections. |
It was rebased a bit more than a month ago |
ghost
merged commit 
This is the first installment of a larger idea that includes LINQ-style stuff (joins, merges and such).
https://d.puremagic.com/issues/show_bug.cgi?id=9616