Fix issue 8085 #987
Fix issue 8085 #987
Conversation
|
Looks good. Is the ODX/64 failure caused by this? |
|
I mean OSX |
|
Given that it osx/64 passes the master branch builds and has never passed this pull request (across many attempts), it's pretty much a given that this pull request either causes or exposes the problem. |
|
Based on the message produced by the assert statement, it seems that osx/64 is somehow doing something funny when returning dchar from string (as a range). Since that's unrelated to this pull request, I've decided to change the unittest so that it operates on dstring from the get-go. (Hopefully this will fix the failing autotest... somebody should look into why dchar conversion from string is acting funny, though.) |
|
Rebased & tidied up commits. |
|
While there's a repro, would you make sure to file a bug report to capture the problem? |
|
Hrmph. Changing it to dstring didn't fix the problem. It's got to be a bug with string/dstring ranges on osx/64. However, I don't have any way of reproducing this 'cos I don't have access to osx/64, so the best I can do is just copy-n-paste the entire code into the bug report. |
|
Alright, I'm sick of this buggy behaviour of strings/dchar on OSX/64 that I have no way to track down. Version out the failing unittest for until issue 9131 is fixed, and replace with another unittest that doesn't depend on strings of any sort. |
|
OK now the autotester passes. Ready to merge! @andralex |
|
merged, thanks |
| if (_items.empty) return; | ||
| if (!_items.front.empty) break; | ||
| } | ||
| _current = _items.front; |
There was a problem hiding this comment.
We'd need to make this (provided we have forward iteration):
_current = _items.front.save;To be able to make joiner a true forward range.
There was a problem hiding this comment.
Actually, that's not enough. If the subranges are themselves ranges of ranges, then their respective subranges may be consumed if the user iterates over the .save'd range.
There was a problem hiding this comment.
Ah... But now we're back to the square one argument: That is just the element type of the "RoR" that we are iterating over. They are just elements, but not part of our range interface. This "third level" range is just as irrelevant to us as the "second level" range is irrelevant to array.save.
We were tasked with transforming the RoR into a R. The fact that those elements are also themselves ranges is not our problem, and shouldn't be our problem. Maybe the user wanted to have a RoR whose elements are mutable char[], which are not meant to be duplicated. What do we know?
There was a problem hiding this comment.
You're right, the .save exposed by joiner only guarantees saving the state of the joined ranged itself, it says nothing about the state of the elements themselves. My bad.
This change makes joiner not assume anything about the persistence of .front. I know there has been no consensus on the transience issue yet, but this change is harmless and makes the code more robust, so I'm submitting it. Plus, it lets us close issue 8085 sooner, regardless of how the transience issue will be decided on.