Skip to content

Implement Array#fetch_values#11555

Closed
byroot wants to merge 1 commit intoruby:masterfrom
byroot:array-fetch-values
Closed

Implement Array#fetch_values#11555
byroot wants to merge 1 commit intoruby:masterfrom
byroot:array-fetch-values

Conversation

@byroot
Copy link
Copy Markdown
Member

@byroot byroot commented Sep 5, 2024

[Feature #20702]

Works the same way than Hash#fetch_values for for array.

Copy link
Copy Markdown
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but should we implement it in Ruby?

@byroot
Copy link
Copy Markdown
Member Author

byroot commented Sep 5, 2024

I though about that way.

I suppose it's just:

def fetch_values(*indexes, &block)
  indexes.map { |i| fetch(i, &block) }
end

@tenderlove
Copy link
Copy Markdown
Member

Since it's a new method, clearly it's not a bottleneck in anyone's app. We could make a primitive call for fetch if we don't want to push an extra frame. I don't have a strong opinion though, implementing in C is fine too (though we'd probably want to convert to Ruby again in the future for YJIT? 🤷🏻‍♀️)

@byroot
Copy link
Copy Markdown
Member Author

byroot commented Sep 5, 2024

#11557

@byroot
Copy link
Copy Markdown
Member Author

byroot commented Sep 5, 2024

Yeah, my concern isn't so much performance but more about potentially yielding at an unexpected point or some other very subtle behavior issues. And also just consistency with Hash#fetch_values which it's modeled on.

But yeah, this method doesn't exactly cry hotspot.

@tenderlove
Copy link
Copy Markdown
Member

But yeah, this method doesn't exactly cry hotspot.

The good part about implementing it from the beginning in Ruby is that if people complain about speed we can rewrite in C (or tell them to use YJIT). 😂

@nobu nobu changed the title Implement Array#fetch-values Implement Array#fetch_values Sep 6, 2024
@nobu
Copy link
Copy Markdown
Member

nobu commented Sep 6, 2024

The commit message is wrong, s/-/_/

[Feature #20702]

Works the same way than `Hash#fetch_values` for for array.
@byroot byroot force-pushed the array-fetch-values branch from e6c1208 to ffc2013 Compare September 6, 2024 06:30
@byroot
Copy link
Copy Markdown
Member Author

byroot commented Sep 6, 2024

Corrected.

@byroot
Copy link
Copy Markdown
Member Author

byroot commented Sep 6, 2024

Merged #11557 instead

@byroot byroot closed this Sep 6, 2024
@byroot byroot deleted the array-fetch-values branch September 6, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants