-
Notifications
You must be signed in to change notification settings - Fork 178
Add support for big ranges #6202
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
base: master
Are you sure you want to change the base?
Conversation
| # length | ||
| gap> f(-2^60,2^60-1); | ||
| Error, Range: the length of a range must be a small integer | ||
| [ Error, Length of range is too large |
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.
looks as if the opening bracket gets printed before the error occurs, is this intended?
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.
No, that's definately a bug. This code still needs quite a bit of polishing, but I was curious if the basic idea looks reasonable.
|
One package which uses ranges is cvec, here is an example of what happens if you try to use large ranges in cvec (that's a horrible error message, need improvement, but basically when cvec tries to get the step of a range as an 'Int', then an error is thrown). |
|
I was not aware of this PR but discussed such changes with @ChrisJefferson on Slack, so here is a summary of my stance: Anyway, from my POV the main obstacle of supporting "large ranges" by modifying the existing
As such I don't find it very appealing to modify the existing [...] [...] there are a lot of things to trip over. Like it now must install a [...] as I wrote, i would not want to [modify or] duplicate Note that there are a few place in the kernel that do special things for |
| @@ -302,18 +412,20 @@ static Obj Elm0vRange(Obj list, Int pos) | |||
| static Obj ElmRange(Obj list, Int pos) | |||
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.
Note that this kernel function only deals with "small" positions.
Is the idea of the "big ranges" in this PR to support ranges where the end points are "large" (= don't fit into immediate ints), but the overall length is still "small" (so that it whole range would still be allowed in the IsSmallList filter) ? Or is it meant to also represent ranges were the length is also large? In the latter case, a lot more needs to be done than what is in this PR.
If one restricts to ranges with large endpoints but small length, I guess this PR could be made to work, but from a user's perspective that seems like a somewhat strange restriction.
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.
In the example that motivated this pull request, both the upper bound and the length are large.
| # Basic large integer ranges | ||
| gap> result := [2^64..2^64 + 10];; |
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.
More interesting: try result:=[1..2^100]; and then try things like Length(result) or IsBound(result[2^100])
This is a test PR, I'm curious how many things break.
This adds support for ranges outside the usual 'small integer' type. It is not finished and likely to break all kinds of things.
Also, many functions are not yet supported with 'large ranges' -- but they should produce clean error messages.
I'm mainly curious if people think this is an interesting direction to take.
FOR INFO: Partly as a test to see how well it works, this was mostly written by 'Claude Code'. I have checked tests initially pass but I haven't carefully checked all the code -- I would obviously do this before a future merge, along with a lot more testing.