Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tuple: support negative index #4735

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Tuple: support negative index #4735

merged 1 commit into from
Jul 26, 2017
+48 −5

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Jul 22, 2017 •
edited
Loading

Close #4714

Now, it works:

tuple = {42, "foo", true}
p typeof(tuple[-1]) # => Bool
p typeof(tuple[-2]) # => String
p typeof(tuple[-3]) # => Int32

i = -1
p typeof(tuple[i]) # => (Bool | String | Int32)
p tuple[i]         # => true

# And, `tuple[-4]` causes a compile error:
# index out of bounds for Tuple(Int32, String, Bool) (-4 not in -3..2)

makenowjust
makenowjust commented Jul 22, 2017
View reviewed changes
src/tuple.cr
@@ -175,6 +175,7 @@ struct Tuple
# tuple.at(3) { 10 } # => 10
# ```
def at(index : Int)
index += size if index < 0
Copy link
Contributor Author

@makenowjust makenowjust Jul 22, 2017 •
edited
Loading

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.

@makenowjust makenowjust force-pushed the feature/tuple/support-negative-index branch from 4bc228e to eb93261 Compare July 22, 2017 03:42
@makenowjust
Copy link
Contributor Author

This feature looks good to everyone for now.

@mverzilli Could you merge this? Or what should I do for merging?

@ysbaddaden
Copy link
Contributor

As always: what are actual use cases?

@makenowjust
Copy link
Contributor Author

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.
And, this makes it easier to use a tuple as like an array.

@RX14
Copy link
Member

RX14 commented Jul 26, 2017

@ysbaddaden consistency with every other indexable?

@mverzilli
Copy link

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.

@RX14
Copy link
Member

RX14 commented Jul 26, 2017

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.

@mverzilli mverzilli merged commit a14f4e8 into crystal-lang:master Jul 26, 2017
@mverzilli
Copy link

@RX14 I generally agree with that opinion, but some times it is good to lean on other people :). Thanks @makenowjust!

@docelic
Copy link
Contributor

docelic commented Jul 26, 2017 •
edited
Loading

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!

@makenowjust makenowjust deleted the feature/tuple/support-negative-index branch July 26, 2017 13:54
@ysbaddaden
Copy link
Contributor

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 Tuple(String, String, ... String) doesn't bring any benefit over a simple Array(String) in your case.

@straight-shoota
Copy link
Member

The language specification recommends tuples for immutability and no heap allocation.

@mverzilli mverzilli added this to the Next milestone Jul 26, 2017
@RX14
Copy link
Member

RX14 commented Jul 26, 2017

@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 freeze like ruby.

@makenowjust
Copy link
Contributor Author

I found this in Indexable document:

Indexing starts at 0. A negative index is assumed to be relative to the end of the container: -1 indicates the last element, -2 is the next to last element, and so on.

straight-shoota pushed a commit to straight-shoota/crystal that referenced this pull request Aug 11, 2017
@makenowjust @straight-shoota
Tuple: support negative index (crystal-lang#4735)
b3b8fb2
Val pushed a commit to Val/crystal that referenced this pull request Aug 12, 2017
@makenowjust @Val
Tuple: support negative index (crystal-lang#4735)
923fc26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardiff bcardiff

@RX14 RX14

Assignees
No one assigned
Labels
kind:feature pr:needs-review status:accepted topic:compiler topic:stdlib
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@makenowjust @ysbaddaden @RX14 @mverzilli @docelic @straight-shoota @bcardiff