From b4213a73b807cf8c8884e29d37308c46ca80352a Mon Sep 17 00:00:00 2001 From: Nobuyoshi Nakada Date: Thu, 14 Sep 2023 22:45:42 +0900 Subject: [PATCH] [Feature #19839] Fix `Range#overlap?` for empty ranges Empty ranges do not overlap with any range. Regarding benchmarks, PR#8242 is significantly faster in some cases, but one of these two cases is a wrong result. | |ActiveSupport| PR#8242|built-ruby| |:--------------------------|------------:|-------:|---------:| |(2..3).overlap?(1..1) | 7.761M| 15.053M| 32.368M| | | -| 1.94x| 4.17x| |(2..3).overlap?(2..4) | 25.720M| 55.070M| 21.981M| | | 1.17x| 2.51x| -| |(2..3).overlap?(4..5) | 7.616M| 15.048M| 21.730M| | | -| 1.98x| 2.85x| |(2..3).overlap?(2..1) | 25.585M| 56.545M| 32.786M| | | -| 2.21x| 1.28x| |(2..3).overlap?(0..1) | 7.554M| 14.755M| 32.545M| | | -| 1.95x| 4.31x| |(2..3).overlap?(...1) | 6.681M| 5.843M| 32.255M| | | 1.14x| -| 5.52x| |(2...3).overlap?(..2) | 6.676M| 5.817M| 21.572M| | | 1.15x| -| 3.71x| |(2...3).overlap?(3...) | 7.392M| 14.755M| 31.805M| | | -| 2.00x| 4.30x| |(2..3).overlap?('a'..'d') | 3.675M| 3.482M| 17.009M| | | 1.06x| -| 4.89x| --- benchmark/range_overlap.yml | 19 +++++++++ range.c | 77 ++++++++++++++++++++++++++++++++++--- test/ruby/test_range.rb | 35 +++++++++++++++++ 3 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 benchmark/range_overlap.yml diff --git a/benchmark/range_overlap.yml b/benchmark/range_overlap.yml new file mode 100644 index 0000000000..700a00053c --- /dev/null +++ b/benchmark/range_overlap.yml @@ -0,0 +1,19 @@ +prelude: | + class Range + unless method_defined?(:overlap?) + def overlap?(other) + other.begin == self.begin || cover?(other.begin) || other.cover?(self.begin) + end + end + end + +benchmark: + - (2..3).overlap?(1..1) + - (2..3).overlap?(2..4) + - (2..3).overlap?(4..5) + - (2..3).overlap?(2..1) + - (2..3).overlap?(0..1) + - (2..3).overlap?(...1) + - (2...3).overlap?(..2) + - (2...3).overlap?(3...) + - (2..3).overlap?('a'..'d') diff --git a/range.c b/range.c index 946da0c4de..f8950f3f29 100644 --- a/range.c +++ b/range.c @@ -2151,6 +2151,18 @@ range_count(int argc, VALUE *argv, VALUE range) } } +static bool +empty_region_p(VALUE beg, VALUE end, int excl) +{ + if (NIL_P(beg)) return false; + if (NIL_P(end)) return false; + int less = r_less(beg, end); + /* empty range */ + if (less > 0) return true; + if (excl && less == 0) return true; + return false; +} + /* * call-seq: * overlap?(range) -> true or false @@ -2161,6 +2173,49 @@ range_count(int argc, VALUE *argv, VALUE range) * (0..2).overlap?(3..4) #=> false * (0..).overlap?(..0) #=> true * + * With non-range argument, raises TypeError. + * + * (1..3).overlap?(1) # TypeError + * + * Returns +false+ if an internal call to <=> returns +nil+; + * that is, the operands are not comparable. + * + * (1..3).overlap?('a'..'d') # => false + * + * Returns +false+ if +self+ or +range+ is empty. "Empty range" means + * that its begin value is larger than, or equal for an exclusive + * range, its end value. + * + * (4..1).overlap?(2..3) # => false + * (4..1).overlap?(..3) # => false + * (4..1).overlap?(2..) # => false + * (2...2).overlap?(1..2) # => false + * + * (1..4).overlap?(3..2) # => false + * (..4).overlap?(3..2) # => false + * (1..).overlap?(3..2) # => false + * (1..2).overlap?(2...2) # => false + * + * Returns +false+ if the begin value one of +self+ and +range+ is + * larger than, or equal if the other is an exclusive range, the end + * value of the other: + * + * (4..5).overlap?(2..3) # => false + * (4..5).overlap?(2...4) # => false + * + * (1..2).overlap?(3..4) # => false + * (1...3).overlap?(3..4) # => false + * + * Returns +false+ if the end value one of +self+ and +range+ is + * larger than, or equal for an exclusive range, the end value of the + * other: + * + * (4..5).overlap?(2..3) # => false + * (4..5).overlap?(2...4) # => false + * + * (1..2).overlap?(3..4) # => false + * (1...3).overlap?(3..4) # => false + * * Related: Range#cover?. */ @@ -2168,15 +2223,27 @@ static VALUE range_overlap(VALUE range, VALUE other) { if (!rb_obj_is_kind_of(other, rb_cRange)) { - rb_raise(rb_eTypeError, "argument must be Range"); + rb_raise(rb_eTypeError, "wrong argument type %"PRIsVALUE" (expected Range)", + rb_class_name(rb_obj_class(other))); } - VALUE beg_self, beg_other; + VALUE self_beg = RANGE_BEG(range); + VALUE self_end = RANGE_END(range); + int self_excl = EXCL(range); + VALUE other_beg = RANGE_BEG(other); + VALUE other_end = RANGE_END(other); + int other_excl = EXCL(other); - beg_self = RANGE_BEG(range); - beg_other = RANGE_BEG(other); + if (empty_region_p(self_beg, other_end, other_excl)) return Qfalse; + if (empty_region_p(other_beg, self_end, self_excl)) return Qfalse; - return RBOOL(rb_equal(beg_self, beg_other) || range_cover(range, beg_other) || range_cover(other, beg_self)); + /* if both begin values are equal, no more comparisons needed */ + if (rb_equal(self_beg, other_beg)) return Qtrue; + + if (empty_region_p(self_beg, self_end, self_excl)) return Qfalse; + if (empty_region_p(other_beg, other_end, other_excl)) return Qfalse; + + return Qtrue; } /* A \Range object represents a collection of values diff --git a/test/ruby/test_range.rb b/test/ruby/test_range.rb index 17d1e3a431..8c01254d19 100644 --- a/test/ruby/test_range.rb +++ b/test/ruby/test_range.rb @@ -1076,7 +1076,42 @@ class TestRange < Test::Unit::TestCase assert_operator(0.., :overlap?, 1..2) assert_operator(0.., :overlap?, 1..) + assert_not_operator((1..3), :overlap?, ('a'..'d')) + assert_raise(TypeError) { (0..).overlap?(1) } assert_raise(TypeError) { (0..).overlap?(nil) } + + assert_operator((1..3), :overlap?, (2..4)) + assert_operator((1...3), :overlap?, (2..3)) + assert_operator((2..3), :overlap?, (1..2)) + assert_operator((..3), :overlap?, (3..)) + assert_operator((nil..nil), :overlap?, (3..)) + assert_operator((nil...nil), :overlap?, (nil..)) + + assert_raise(TypeError) { (1..3).overlap?(1) } + + assert_not_operator((1..2), :overlap?, (2...2)) + assert_not_operator((2...2), :overlap?, (1..2)) + + assert_not_operator((4..1), :overlap?, (2..3)) + assert_not_operator((4..1), :overlap?, (..3)) + assert_not_operator((4..1), :overlap?, (2..)) + + assert_not_operator((1..4), :overlap?, (3..2)) + assert_not_operator((..4), :overlap?, (3..2)) + assert_not_operator((1..), :overlap?, (3..2)) + + assert_not_operator((4..5), :overlap?, (2..3)) + assert_not_operator((4..5), :overlap?, (2...4)) + + assert_not_operator((1..2), :overlap?, (3..4)) + assert_not_operator((1...3), :overlap?, (3..4)) + + assert_not_operator((4..5), :overlap?, (2..3)) + assert_not_operator((4..5), :overlap?, (2...4)) + + assert_not_operator((1..2), :overlap?, (3..4)) + assert_not_operator((1...3), :overlap?, (3..4)) + assert_not_operator((...3), :overlap?, (3..)) end end