Support Tuple#[](Range) with compile-time range literals#10379
Merged
asterite merged 3 commits intocrystal-lang:masterfrom Feb 17, 2021
Merged
Support Tuple#[](Range) with compile-time range literals#10379asterite merged 3 commits intocrystal-lang:masterfrom
Tuple#[](Range) with compile-time range literals#10379asterite merged 3 commits intocrystal-lang:masterfrom
Conversation
asterite
reviewed
Feb 10, 2021
| def codegen_tuple_indexer(type, value, index : Range) | ||
| case type | ||
| when TupleInstanceType | ||
| tuple_types = type.tuple_types[index].map &.as(Type) |
Member
There was a problem hiding this comment.
It's so convenient that implementing [] with range is actually done by using [] with a range :-)
When I initially thought about this I thought it would be more complex because of all the indexing logic involved... but it's already there!
asterite
reviewed
Feb 10, 2021
asterite
reviewed
Feb 10, 2021
asterite
reviewed
Feb 10, 2021
Comment on lines
+209
to
+211
| def [](range : Range) | ||
| {% raise "Tuple#[](Range) can only be called with range literals known at compile-time" %} | ||
| end |
Member
There was a problem hiding this comment.
I think this is fine for now. But eventually returning an Array is fine too.
asterite
approved these changes
Feb 10, 2021
Member
asterite
left a comment
There was a problem hiding this comment.
This is great!! I left a couple of optional suggestions.
straight-shoota
approved these changes
Feb 10, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tuple#[](index : Int)returns the element in the proper type ifindexis an integer literal known at compile-time. This PR adds aTuple#[](range : Range)pseudo-overload that only supports compile-time range literals, and returns the elements in a proper sub-tuple type:The motivation is to enable #132 for
Tuplevalues on the RHS, without creating temporaryArrays: (only one layer of decomposition is considered here)It is assumed that the Crystal compiler should transform the above multi-assign expression into the following, in a purely syntactic manner:
The method supports incomplete and exclusive ranges to match the behaviour of other
#[](Range)methods, even though the expansion here doesn't require them.The range index corresponding to the splat variable is always known at compile-time, because it can be deduced from the number of non-splat variables before and after it; thus it suffices to implement the special case for
Tuples where this range is a constant expression. (The same syntactic transformation already works out of the box for types that implement#[](Range), likeArray.)On the other hand, the only sensible return type of
Tuple(*T)#[](Range)for runtime range arguments isArray(Union(*T)). This PR does not address this discrepancy at all, that's left to #7619. Method lookup bypasses prelude definitions of(Named)Tuple#[]if a compile-time indexer is found, so the compile-time error seen in the top example will not be triggered by constant range literals.The documentation for this method does not take #10243 into account.
Also see the Gitter discussion that led up to this.