New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements to hasSlicing #854
Conversation
Of interesting note, I just figured out that these changes make it impossible to define slicing on a type which wraps an infinite range, because slicing the infinite range would mean getting the result of
So, this could definitely break some code involving infinite ranges, but if we want to be able to have the guarantee that a slice of a finite range is reassignable to the original (as code often assumes), I think that we're stuck. And not having that guarantee for finite ranges will definitely be problematic. |
LGTM, regarding the finite ranges. Regarding the infinite ones, I've been (silently) working on my "isDropable" development. It is looking good, and I think it can fix all the issues you have been running into. I've just been more busy fixing things then inserting new things.
This allows two things:
At this point, we could add a function in range akin to using IMO, "hasSlicing" should mean that it is re-assignable to the original range, no exception. Do you think it would be at all possible to post-pone the "infinite" part of this enhancement, so I can present my developed solution? Everything else, though, LGTM. |
Based on your description, I don't see how it solves any problems for slicing infinite ranges. In order for them to function the same way that slicing does with finite ranges, the slice must always assignable to the original, and that's never going to work, because the slice is inherently finite whereas the original is infinite. The only thing from your proposal that's immediately relevant to this pull request as far as I can see is the idea that infinite ranges not ever be sliceable, which is arguably a good idea. I'm not particularly fond of the idea that they be sliceable, particularly since it screws up the guarantee that sliceable ranges are always reassignable to the original. But even if we take that tact, it doesn't really conflict with my changes. It just means that the slicing on infinite ranges is removed later. It wouldn't have any effect on finite ones. So, I think that we can look at doing what you propose later without affecting what I'm proposing here. However, as much as I really like the idea of making infinite ranges non-sliceable, Andrei seems to like the idea of them being sliceable, so I don't know how easy it will be to change that. By the way, it should be |
Well, as you said, it would solve the problem by saying that an infinite range simply cannot be slice-able. That's my major point. My proposal is to have a functionality which can operate in a sensible way on infinite ranges (and normal ranges), without subverting the definition of
Well, I'd argue you are now inserting formal support for slicing infinite ranges, when the support was just implicit. Your changes that make it that anything not back-assignable would not be slice-able, except for infinite ranges. I say f'em, and break them along with all the other ranges you will be breaking.
|
No. It's officially supported right now. It's just that
As I said, that's a separate issue. As long as slicing infinite ranges is legal (as it currently is), my changes are relevant. They only become irrelevant if make slicing infinite ranges illegal, which I suspect that you're going to have a hard time talking Andrei into. You're going to need a very solid proposal, especially when it sounds like what you're proposing complicates things a fair bit. It may be worth doing, but you probably have an uphill battle in convincing Andrei of that. And as long as Andrei hasn't been convinced that infinite ranges shouldn't be sliceable (whether your proposal is accepted or not), then these changes are quite relevant. Regardless, I'd suggest that you organize your proposal and bring it up for discussion in the newsgroup. Until you've made a solid case for it and convinced Andrei of it, it's going nowhere no matter how good it may be. |
You are right. Never mind then and thanks for the replies. I'll formalize my proposal and present it to the newsgroup. |
The only argument I see for not making the changes to infinite ranges here is to just not require that they use |
Yes, I think you are right. I'll make this my P0, and bring it up after you pull goes through. |
Is this good to merge or are there unresolved concerns? |
Given the nature of the change, I think that Andrei really needs to approve it. Based on previous discussions with him, I think that he's okay with it (at minimum with the changes to finite ranges - I don't know about the infinite ones), but this is the sort of thing that he should probably look at (since it's a fundamental change to one of the major range primitives). |
Assigning @andralex. |
Could I get an opinion on the related : This is I'm hitting the issue in this pull request: #837 , which [improves/fixes] emplace to require strict construction. In my specific pull, the problem is actually bug related, but it does raise the question: Do you think that line of code in Phobos is valid? |
I think we should simplify. Sliceable means ctor AND assignment, no ifs and buts. If some of such are not required, fine, just that such rare ranges can't claim to be sliceable. |
So, you're saying that both of these lines should compile?
the problem the second one is that you then can't slice a But if you're suggesting that we ignore the possibility of slicing a Perhaps we should make it so that |
Looks like I had understood it backwards actually. Construction back to the original type should be fine, for all the arguments you put forward. |
@@ -920,7 +920,7 @@ unittest | |||
InputRange range; | |||
fill(range,filler); | |||
foreach(value;range.arr) | |||
assert(value == filler); | |||
assert(value == filler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: The assert was inside a foreach, so the indentation was actually correct... Unless I missed something.
Okay. It now tests for both construction and assignment from the slice. A potentially major side effect of this is that const and immutable arrays will no longer be considered sliceable (though their slices will be sliceable, since they're tail-const). We should probably add an eponymous template which tests for whether a type can be fully sliced (that is you can slice it without indices - The one thing that I'm still a bit divided on here is slicing infinite ranges. It would be kind of nice to get rid of that in favor of something else given that they really don't follow the normal slicing rules, and if we do replace it with something else (e.g. monarchdodra's |
I was kind of waiting for this to pull to go through, but it is ready. I ask the group today then. |
I don't really see any reason to delay discussing your proposal until this pull is merged. The issues are related, but I don't see how what you're proposing would be dependent on this pull request. |
Okay, based on the newsgroup discussion on
to Given that infinite ranges don't define |
static if(is(typeof(r[0 .. $]))) | ||
{ | ||
R t = r[0 .. $]; | ||
t = r[0 .. $]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go with the more restrictive but simpler static assert(is(typeof(r[0 .. $] == R));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that you want to check that the slice is the original type exactly? That's not necessarily true with arrays, but as long as the first part still checks for construction and assignment, then the array can't be const or immutable, in which case checking that the slice using $
is the exact type should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for the type of the slice being exactly the same as the original doesn't work. I suspect that it's a compiler bug, but I don't know. In particular, iota
fails with hasSlicing
if I make that change. Asserting static assert(is(typeof(r[0 .. $]) == R))
, static assert(is(typeof(r[0 .. $ - 1]) == R))
, or static assert(is(typeof(r[0 .. 2]) == R))
results in
Error: static assert (is(Result == Result)) is false
However, the code currently in the pull request works. So, I'm inclined to just go with what's here for the moment. And if we make it more restrictive later, it probably won't break much code (if any), since the main reason for a slice to be a different type would be is if it's a tail-const version of the original range, and that would presumably only be done if the range itself were const
. And what's here tests what we really care about. The slice doesn't really need to be exactly the same type as the original so long as it assigns to the original and the original is constructible from it. It's not the same for arrays, after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bug. It's exactly the one I came asking about above.
So, status on this? |
@andralex Is there any reason not to merge this at this point (assuming that it passes the pull tester on its next run)? The one remaining suggestion/issue that I'm aware of was you suggesting to use the more restrictive I did, however, remove the checks for whether a slice could be a different type from the original and still convert to it, so while this version still technically allows it, it's no longer checked for, and if/when we make |
Just want to post this bug entry: Someone being bit in the ass by the issue you are fixing. It makes for an interesting "real-world" example of what's out there. |
Okay. I adjusted the comments to |
I would like to point out that this pull does fix a rather nasty regression (issue #8556), so it either needs to be merged before the next release, or a version of it without the changes to |
Okay. I just rebased again, so it should be mergeable now. Hopefully this gets merged before yet another change to std.range gets merged in. |
//static assert(is(typeof(r[0 .. $ - 1]) == R)); | ||
R u = r[0 .. $ - 1]; | ||
u = r[0 .. $ - 1]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, this kind of opDollar verification feels like it should also be added to isRandomAccessRange, no?
static if (is(typeof(r[$])))
{
static assert(is(typeof(r.front) == typeof(r[$])));
static assert(is(typeof(r.front) == typeof(r[$ - 1])));
}
Thoughts?
It's related, but it's a stand alone change, so I can do it if you don't feel like weighing down this already long an important pull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd prefer that this just got merged in soon rather then messing with it further. I think that the main reason that it hasn't is that Andrei has been too busy to look at it again. But whether such a change is included or not wouldn't really affect it much, I would think.
I agree that isRandomAccessRange
should be changed as you suggest, but if you do that before this is merged, I'll have to rebase this pull yet again, so I'd kind of prefer that you didn't make such a change. I may just add it to this pull this evening if it doesn't get merged in first (which I assume it won't be), since I don't think that such a restriction is particularly controversial, and it is related to these changes. But we'll see if I have time or not (or maybe I'll get improbably lucky and this will get merged first).
What I'd really like to see is issue# 7177 implemented so that we can just require that $
work, as we could then increase the restrictions of hasSlicing
all at once. But that requires convincing a compiler dev to do that, and Kenji didn't seem to think that it was a good idea when it came up in a pull request discussion recently. So, maybe a discussion on that should be brought up in the newsgroup or something.
But there's no reason that I'm aware of to delay this pull request at this point other than Andrei being too busy to look it over again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, drat. A change was made to std.algorithm, and I'm going to have to rebase this again. I'm getting tired of that.
I'll just add the extra restriction to isRandomAcessRange
when I do that (hopefully this evening).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that. Ping me when done. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andralex Okay. I rebased and added the extra requirement to isRandomAccessRange
that r[$]
work properly if it compiles with the range (though we can't require that it work for all random-access ranges without enhancement #7177 being implemented, just that it does have the correct type if it does compile).
@andralex ping? |
@@ -7866,8 +8017,7 @@ assert(buffer2 == [11, 12, 13, 14, 15]); | |||
|
|||
|
|||
/++ Ditto +/ | |||
static if(isBidirectionalRange!R) | |||
void popBack() | |||
static if(isBidirectionalRange!R) @property void popBack() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect @property
attribute addition to popBack
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A botched rebase apparently, as I wasn't doing anything to popBack
on anything. I'll fix it shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. It should be fixed now.
…ngth." This reverts commit 6a0b6c4.
takeExactly now returns the same type as take where possible, and neither take nor takeExactly check hasSlicing for infinite ranges (since infinite ranges will soon be required to use them for slicing, and that creates a circular dependency among those 3 templates
Now, it enforces that opSlice returns a range which can be assigned to the original range type (so that it can be assigned to the original range as long as the original range isn't const) as long as it's finite, and it enforces that opSlice's result is the result of take when the range is infinite.
import std.algorithm; import std.range; void main() { auto N2 = sequence!"n"(cast(size_t)1).map!"a"; } ceased to compile, because Map's opSlice won't work anymore over infinite ranges, because the result can't be reassigned to the original.
Ideally, opDollar would be outright required for any range with slicing, but unless/until it's changed so that length automatically aliases to opDollar when opDollar isn't defined (issue# 7177), that's probably not a reasonable requirement to make.
The extra requirements are not currently enabled because of bug# 8847, but they're now there, and they're listed as their in the documentation so that no one will think that they're not supposed to apply.
It now requires that indexing with $ result in the same type as front if r[$] works, and if that works, and the range isn't infinite, r[$ - 1] must be the same type as front. Ideally, we'd require that r[$] work regardless, but without enhancement dlang#7177 being implemented, that would likely break too much code, as opDollar was only recently fixed and probably isn't used much.
They were removed previously but reintroduced due to a bad rebase, so I'm removing them again here.
Now I have posted a compiler fix dlang/dmd#1380 to fix a bug which reported in issue 8556. It correctly breaks current Phobos build, and this pull can fix the forward reference errors around hasSlicing. But, sorry, I am not familiar with the recent change of Phobos modules. So I'd like to wait @andralex 's reviewing. |
R r = void; | ||
|
||
static if(isInfinite!R) | ||
typeof(take(r, 1)) s = r[1 .. 2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that r[1 .. 2]
must be of the type typeof(take(r, 1))
for infinite ranges. So, using auto
would defeat the purpose of the test.
merged, thanks |
Thanks! Now if we could only have issue# 7177 implemented, then we could fully tighten |
Great! The only question: where are the breaking change notices in changelog? |
They'll be put there. I never put changes to the changelog in pull requests, because frequent rebases are then required. I do better at getting notices in there then most of the other Phobos devs. So, you don't need to worry about that. Thanks for the reminder though. |
By the way, I have never knew how to update chanelog (as there are lots of chanelog files). IMHO, some examples/instructions for contributors will be useful. |
I think the (new) wiki is the perfect place for this. |
|
||
s = r[1 .. 2]; | ||
|
||
static if(is(typeof(r[0 .. $]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for resurrecting this, but I figure this is the best place to discuss it:
Shouldn't we make the r[0 .. $]
mandatory for an infinite ranges to satisfy hasSlicing
?
Rationale is:
- Infinite ranges with hasSlicing is new, so we shouldn't be breaking code.
- The compiler will simply never be able to automatically generate opDollar for them.
- Range adapters that want to implement slicing for infinite ranges must rely on the range providing
r[i .. $]
.
This is because the adaptor cannot return the type Adaptor!(Take!Range)
, as it has to return the type Take!(Adaptor!Range)
to verify has slicing (this has actually turned out to always be a good thing, in my short experience with this).
Once you take this into account, the only way to implement the adaptor's opSlice
operation is to "fast forward" the internal range (r = r[i .. $]
), and then return this.take(j - i)
.
either that or remove the static condition of typeof(take(r, 1)) s = r[1 .. 2];
. But I think this requirement is both brilliant for template programming, and the requirement actually forces you to use some very clean implementations.
This question is asked in the context of a zip of slice-able infinite ranges.
If the opSlice
operation returns a Zip!(staticMap(Ranges, Take))
, then zip will fail the hasSlicing
condition. It must return a: Take!(Zip!Ranges)
(this is a good thing actually: Take of zip should be much more efficient than zip of takes).
Problem: As explained above, to implement such slicing, zip will need to first slice the internal ranges from i to $. Testing if a range has opDollar is already kind of complicated, and heavily complexifies the implementation (did you see my "chunks" improvement?).
In the context of multiple ranges, just writing the condition of static if ("all ranges are infinite and all of them provide slice to end")
is close to nightmarish...
Thoughts? We can take it to newsgroup if you feel it requires more discussion.
The primary goal of this pull request is to tighten up
hasSlicing
, since it's too lax. The changes to it are:opSlice
must be implicitly convertible to the original to allow you to assume that the slice is reassignable to the original (except when the original wasconst
orimmutable
, in which case it can be assigned to a new variable of the same type as the original). It's implicitly convertible rather than exact so that the slice can be tail-const.opSlice
must be the result oftake
ortakeExactly
(they now return the same type for infinite ranges).opSlice
is required to be a forward range rather than an input range. As slicing is essentially the same as callingsave
except for the fact that the result covers fewer elements, it makes no sense for the result to not be a forward range.opSlice
must definelength
as it makes no sense to have a slice with an unknown length. The original doesn't have to, even for finite ranges, because it really has no effect onopSlice
whether the original defineslength
or not. But because the result must havelength
and must be assignable to the original (for finite ranges at least), it probably wouldn't make sense for the original not to havelength
if it's finite. It seemed pointless to check for it though.Also, in order to facilitate using
take
ortakeExactly
for slicing infiinite ranges,take
andtakeExactly
have been adjusted. The main difference is that now neither checkshasSlicing
on infinite ranges (that causes a circular dependency which triggers bug# 8556), buttakeExactly
has also been adjusted to return the same type astake
wherever possible (i.e. when the range is infinite or defineslength
).