Issue Details (XML | Word | Printable)

Key: GROOVY-1553
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Minor Minor
Assignee: Paul King
Reporter: Edwin Tellman
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
groovy

I fixed some problems with ranges and added some additional unit tests.

Created: 14/Nov/06 06:06 AM   Updated: 01/Jul/07 01:10 AM
Component/s: groovy-jdk
Affects Version/s: None
Fix Version/s: 1.1-beta-2

Time Tracking:
Not Specified

File Attachments: 1. Text File empty_range.patch (20 kB)
2. Text File range.patch (77 kB)

Environment: Max OS X 10.4

Testcase included: yes


 Description  « Hide
I added some additional unit tests and JavaDoc for ranges and fixed some problems that I found.

I added step() to the Range interface. It was present in both ObjectRange and IntRange but not in Range. Guillaume approved this change via email.

Hash codes were being computed using only "reverse," "from," and "to." This results in hash codes that are inconsistent with the contract specified by List.hashCode(). As a result, range.equals(list) was true but range.hashCode() == list.hashCode() was false. I removed IntRange.hashCode() and ObjectRange.hashCode() so the use AbstractList.hashCode() instead. This fixes this problem although it is less efficient, especially for large ranges.

I added a checks for null in equals(). equals(null) should return false rather than throwing a NullPointerException.

I made subList(i, i) return an empty list instead of a single element list. subList() is from a to b, exclusive, so it should return an empty list in this case.

I made contains(Object) consistent. IntRange had been returning true if the argument was an element of the range or was a sub-range. ObjectRange had a contains(Comparable) but no contains(Object). ObjectRange.contains(Comparable) returned true if the argument was between from and to and didn't consider sub ranges. The expected behavior, according to the List interface is to return true if the argument is an element of the list. RangeTest tested for this behavior in ObjectRange, making sure a range of BigDecimals from 1.1 to 9.1 didn't contain 8.0. But the test only passed because ObjectRange.contains(Comparable) wasn't called because AbstractList.contains(Object) was called instead.

Anyway, since the List contract says contains(Object) should return true if and only if the argument is a member of the list, I made IntRange.contains() do this and removed ObjectRange.contains(Comparable). I also added IntRange.containsAll() which overrides AbstractList.containsAll() to look for sub ranges efficiently.

I made ranges of Shorts and Floats promote their 'from' value to Integer and Double, respectively. Previously, the from value had been of the original type and all the remaining values the promoted type, as the remaining values were created by incrementing the from value. I think the original version had ClassCastException problems when it tried to compare from with other values in the range. Also, having all values in the range the same type seemed to be more consistent and make more sense.

I removed some uses of InvokerHelper.compareEqual() to be consistent with list. I think compareEqual() is both more and less forgiving than equals(). I think it says Long(0) is equal to Integer(0), for example. Other lists, however, use equals() and equals() is supposed to be symmetric. So, using compareEqual() you get situations where range.equals(list) but !list.equals(range). Also, compareEqual() is sometimes less forgiving than equals(). I think asking a range of integers whether it contains the string "hello", for example, threw a NumberFormatException instead of returning false.

RangeTestSuite runs all the range-related tests.

This issue replaces GROOVY-1546. Please disregard the patches in that issue.



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Edwin Tellman added a comment - 18/Nov/06 02:54 AM
This patch contains unit tests and bug fixes for the EmptyRange class.

The changes to EmptyRange.java are:

EmptyRange extends AbstractList so some methods have been removed from EmptyRange as the versions provided by AbstractList are fine.

get() now throws IndexOutOfBounds exception instead of returning null.

Methods which attempt to modify an EmptyRange all consistently throw OperationNotSupportedException, as EmptyRanges are immutable.

step() returns a new ArrayList each time it is called. Previously EmptyRange kept a list in a member variable and always returned the same list. The problem with this approach is that values may be added to the returned list so the list returned on a subsequent call to step() might not be empty.

I added some JavaDoc.

I added the range tests to the AllGroovyJavaTestSuite.


Paul King added a comment - 11/Jun/07 03:58 AM
Patches applied to HEAD plus a new method containsWithinBounds() added to the Range interface in case anyone was relying on the earlier behaviour. So, if r = 0.0 .. 5.0 then r.contains(2.5) will be false and r.containsWithBounds(2.5) will be true.

Paul King added a comment - 01/Jul/07 12:41 AM
Spotted another problem.

Paul King added a comment - 01/Jul/07 01:10 AM
amendment to GROOVY-1553: 2.0f in 1.0..3.0 now works