GROOVY-7877: The Range abstraction could support numeric ranges where…

[paulk-asert]

… the items in the range differ by some step size different to 1

[paulk-asert]

A spike showing an alternative to PR#360. Basically it takes some of the optimizations desired for ObjectRange from that PR but only implements them in a new class NumberRange. It does also change the default class for creating ranges involving two (non-Integer) Numbers to use NumberRange instead of ObjectRange.

The main thing which is supported is fast calculation of range items rather than relying on next() calls and ranges with a non-integral step size like this:

def r = (0.1..0.9).by(0.1)
assert r == [0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9]

It is a breaking change in that some weird edge cases involving metaprogramming on Numbers will behave differently. There is a slightly better backwards compatibility story than PR#360 however since users are still free to create ObjectRange's manually if they need the old behavior.

I call it a spike because I still have some questions on the current design. For instance, with ObjectRange we have the following behavior:

def or = 'a'..<'f'
assert or.step(2) == ['a', 'c', 'e']
assert or.step(-2) == ['e', 'c', 'a']

But currently inclusivity is remembered and applied to the 'to' end of the range when going in different directions, e.g.

def nr0 = (1..<6).by(2)
assert nr0.step(1) == [1, 3, 5]
assert nr0.step(-1) == [6, 4, 2]

def nr1 = (1..<6).by(-1)
assert nr1.step(2) == [6, 4, 2]
assert nr1.step(-2) == [1, 3, 5]

def nr2 = (1..<6).by(1)
assert nr2.step(1) == [1, 2, 3, 4, 5]
assert nr2.step(-1) == [6, 5, 4, 3, 2]

This may seem unintuitive at first but is kind of useful once understood. It is also somewhat essential to creating efficient ranges that can be traversed in either direction. But it's not the only possible design. We could brute force calculate the final to value (going in the forward direction) and use that as the from value when iterating in reverse.

Another question as brought up by @tkruse in an earlier PR was whether for inaccurate number types like float whether it is better to continually add the step values when iterating or multiply by an index - giving at most two rounding errors rather than a compounding scenario. I also haven't tried to fast calculate all possible cases - it might be possible to cover additional cases. But that can be done I believe as a post optimization. It might also be possible to precalculate the number of items within the range during iteration which might save the cost of comparisons for each iteration.

Also, there are some other optimizations possible. The expression (0.1..0.9).by(0.1) creates two NumberRanges. The first with a step size of one, the second with the desired 0.1 step size. This is just a convenience to piggy back on existing syntax features. It would be possible to enhance the compiler to detect cases like that and instead just create a single NumberRange. Again, I consider this an optimization for later.

[jwagenleitner]

@OverRide

[paulk-asert]

Added, thanks.

[jwagenleitner]

Just curious, why synchronized for the iterator? I see the same on the ObjectRange iterator but can't understand why it would be needed.

[paulk-asert]

It isn't strictly necessary. It is providing a thread-safe iteration mechanism. This is something that was introduced recently in ObjectRange and I just carried it over.

[jwagenleitner]

I figured it was carried over from ObjectRange. I just couldn't figure out why you'd want 2 or more threads to share an iterator. Figured the typical use would be in a foreach loop and each thread would get their own instance of an iterator. Otherwise, if sharing an instance a call to hasNext() followed by next() seems like it could return null for one of the threads after another thread got the last Number.

[paulk-asert]

We could possibly just have a safeIterator method which returned the thread-safe version and not bother normally.

[jwagenleitner]

To me it seems like a true thread-safe iterator would need to lock in hasNext() and unlock when it returns false (or throws) or after a subsequent call by the same lock-holding thread to next(). Though I haven't test it, it appears with these synchronized methods that a thread could have hasNext() return true and a subsequent call to next() return null (though maybe should be NoSuchElementException).

But I still do not understand the benefit of having a thread safe version of the iterator that multiple threads can share and wonder if maybe those synchronized methods were put in by mistake (especially given the nearby comments about it not being thread safe). Also, I don't see similar synchronization in the IntRange class.

[jwagenleitner]

and wonder if maybe those synchronized methods were put in by mistake (especially given the nearby comments about it not being thread safe).

Meant to say put in by mistake on the ObjectRange class originally.

[jwagenleitner]

Minor thing, but should the null check come first.

[paulk-asert]

I removed the slightly flawed attempts at synchronization and adopted a more Java behavior when calling next by throwing a NoSuchElementException once a NumberRange iterator is exhausted.

[jwagenleitner]

Looks really good, nice work.

Just my opinion, but I would not introduce the additional public API alternatives for equals/hashCode until a need presents itself. I think the doc in there stating instances of the class are not good candidates for hashed keys is good and sufficient.

[jwagenleitner]

Forgot to ask if it would be worthwhile to add @since tags on the new IntRange method and on the NumberRange class.

[paulk-asert]

Yes, those additional methods do kind of represent possible premature feature introduction. I'll remove them (or at least comment them out) and I'll add the @SInCE tags too.

[paulk-asert]

Proposed PR merged.