-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Tuple: support negative index #4735
Tuple: support negative index #4735
Conversation
@@ -175,6 +175,7 @@ struct Tuple | |||
# tuple.at(3) { 10 } # => 10 | |||
# ``` | |||
def at(index : Int) | |||
index += size if index < 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.
The reason that I don't use Indexable#check_index_out_of_bound
here is type inference works correctly.
If I rewrite it by using check_index_out_of_bouds
, this method will become like below:
def at(index : Int)
index = check_index_out_of_bounds { return yield }
{% for i in 0...T.size %}
return self[{{i}}] if {{i}} == index
{% end %}
raise "BUG: unreachable"
end
The last raise
is important. Without this, at
method result type is inferred as nilable. And return yield
inside block looks bad to me, so I don't use check_index_out_of_bounds
.
4bc228e
to
eb93261
Compare
This feature looks good to everyone for now. @mverzilli Could you merge this? Or what should I do for merging? |
As always: what are actual use cases? |
For example, this makes it possible to get the last element without union type from a tuple. When want to create the method whose last argument have a meaning, this feature must be needed. |
@ysbaddaden consistency with every other indexable? |
I also have some difficulties seeing a use case for this. I guess it doesn't harm to have it for the sake of consistency and polymorphism with indexables. I'll collect some other opinions from the team and get back to this. |
I don't think everything in the stdlib needs a concrete obvious usecase to be accepted. Tuple is currently an indexable collection. Every other indexable accepts negative index, so tuple should too. Consistency is important too. |
@RX14 I generally agree with that opinion, but some times it is good to lean on other people :). Thanks @makenowjust! |
I had a specific case where I had a Tuple with weekdays: t = { "SUN", "MON", "TUE", ..., "SAT"} And user configuration where e.g. a day specification of -1 would automatically mean the last day in the list, "SAT". Indexing with -1 is not the only way to get this value, of course, but is sure the most straightforward/intuitive/consistent. Thanks! |
Not a big deal, but I'm still dubious of actual use cases: a Tuple is a specific type, with expected elements at an expected position, not a generic collection like Array or StaticArray. I don't believe consistency with other collections is enough of an argument. @docelic thanks for the example, but that's just my point: a |
The language specification recommends tuples for immutability and no heap allocation. |
@ysbaddaden I think you're wrong. Using a tuple for constant arrays is a good idea, because we know for a fact that they will be immutable. Storing an Array means it can be modified, we don't have |
I found this in
|
Close #4714
Now, it works: