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

Allow `Range#include?` on TWZ ranges #31081

Merged
merged 1 commit into from Nov 9, 2017

Conversation

Projects
None yet
5 participants
@pixeltrix
Member

pixeltrix commented Nov 7, 2017

In #11474 we prevented TWZ ranges being iterated over which matched Ruby's handling of Time ranges and as a consequence include? stopped working with both Time ranges and TWZ ranges. However in ruby/ruby@b061634 support was added for include? to use cover? for 'linear' objects. Since we have no way of making Ruby consider TWZ instances as 'linear' we have to override Range#include?.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 7, 2017

@rafaelfranca @matthewd wdyt?

Previously I would've said just use cover? but since Ruby now allows include? it seems to make sense to match it's behaviour. In an ideal world Ruby would have some way of indicating that an object is linear rather than us having to override include?.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 7, 2017

Related, I've just submitted this to finally remove Time#succ:

https://bugs.ruby-lang.org/issues/14091

@jeremy

jeremy approved these changes Nov 7, 2017

@al2o3cr

This comment has been minimized.

Contributor

al2o3cr commented Nov 7, 2017

@pixeltrix Probably just me being paranoid, but is there a performance impact to this? It changes Range#include? from compiled to interpreted.

@matthewd

This comment has been minimized.

Member

matthewd commented Nov 8, 2017

In an ideal world Ruby would have some way of indicating that an object is linear rather than us having to override include?

+1 this seems worth pursuing. Personally it seems to me that any object which doesn't respond to succ but is Comparable should be linear for that purpose... but there are plenty of types that, like Integer, could safely be treated as linear despite supporting succ -- so an explicit opt-in probably does make more sense, as well as being more a conservative change.

We don't even need to look at third party libraries to find them... Date and IPAddr come to mind.

(Really I think that at least === and probably also include? should automatically mean cover? for every range of Comparable (including String), and the fact they don't is an historical accident that should be righted instead of compounded. But I don't see that thought going anywhere.)


To this patch:

Do we need === too?

I'm conscious that is_a?(::Time) feels quite directly redundant with what already happens in core. Maybe we should explicitly check for TWZ?

@al2o3cr we are adding an extra [ruby] method invocation, so it will certainly make it slower to call... but all the important work is still in C-land. If someone's in a truly hot path, they could fall back to using cover? directly.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 8, 2017

@al2o3cr we're already overriding include? to add support for passing a range so any optimisation from not overriding has been lost already (don't think it's optimised in the VM anyway like [],succ, etc. are)

@matthewd good call on === - I'll add it

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 8, 2017

@matthewd no need for === since range.c just defines it as this:

static VALUE
range_eqq(VALUE range, VALUE val)
{
    return rb_funcall(range, rb_intern("include?"), 1, val);
}
Allow `Range#include?` on TWZ ranges
In #11474 we prevented TWZ ranges being iterated over which matched
Ruby's handling of Time ranges and as a consequence `include?` stopped
working with both Time ranges and TWZ ranges. However in
ruby/ruby@b061634 support was added for `include?` to use `cover?` for
'linear' objects. Since we have no way of making Ruby consider TWZ
instances as 'linear' we have to override `Range#include?`.

Fixes #30799.
@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 8, 2017

Changed is_a? checks to TimeWithZone from Time for both this patch and the each & step overrides.

@pixeltrix pixeltrix merged commit eae65ac into master Nov 9, 2017

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pixeltrix pixeltrix deleted the allow-include-time-with-zone-range branch Nov 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment