-
-
Notifications
You must be signed in to change notification settings - Fork 74
Implement lastindex for ragged arrays
#501
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
12c7a0c
implement lastindex for ragged arrays
JoshuaLampert 957641e
add more tests
JoshuaLampert 46ce779
Merge branch 'master' into lastindex-ragged
JoshuaLampert e5a9c0b
add more tests
JoshuaLampert 36561a0
avoid type parameter
JoshuaLampert 40fc321
use fields instead if tuple
JoshuaLampert d1c9ae7
Merge branch 'master' into lastindex-ragged
JoshuaLampert 7d9e008
make lastindex type stable by always returning RaggedEnd
JoshuaLampert 5cf0f66
format
JoshuaLampert File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does this not give inference issues?
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.
Hm yes, might be. As I said, this was AI-generated. I'll see if I can fix that.
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.
instead of using types if it's runtime, so enum then I think the strategy is fine.
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'm not sure I understand.
RaggedEndshould be anenuminstead of a struct? If yes, what would be the values?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.
or just 0 and interpret what zero means.
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.
To be honest, I only understand half of what is going on here, but aren't we loosing information (the
d) when we return 0 instead ofRaggedEnd{d}()? I tried making it work with the help of Claude, but there were always tests failing.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.
yeah I see. The point though is to just make it be some kind of runtime value instead of a compile time value. So instead of a type with d, just like a tuple (true, d) for ragged true/false.
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.
Ok, thanks for the clarification! So, should
Base.lastindexalways return a tuple then? Also in the non-ragged case?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 made a suggestion in 36561a0. This uses runtime values, but still returns
RaggedEnd. As far as I understand we cannot return tuples because we need to overload+and-for that, which we obviously do not want to do for a general tuple.