[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-23 Thread jat 

Slightly different fix committed to trunk at r8308.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-22 Thread markovuksanovic 

Here are some more comments about the things that rice pointed out.


http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode199
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:199:
public void testSubList() {
@rice. Why do you think this would fail? I have simulated this in 1.5
compliance environment and this worked as expected.

On 2010/06/21 20:40:24, Dan Rice wrote:

I think this test will fail (passes in JDK 1.6):



ArrayListString wrappedList = new ArrayListString();
wrappedList.add(A);
wrappedList.add(B);



ListString sub = wrappedList.subList(1, 1);
sub.remove(A); // no A in range



// original list is unchanged
assertEquals(2, wrappedList.size());
assertEquals(A, wrappedList.get(0));



http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode212
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:212:
assertEquals(testList, Arrays.asList(2, 6, 4));
(E remove(int index)) returns the removed element.
(boolean remove(Object o)) returns true if the list contained element.
Otherwise false.

The first definition is applied here. I think there's nothing wrong with
this test case.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-22 Thread rice 

I think we need a few more tests to be sure that sublists are working as
intended.  If things are problematic after adding more tests, another
approach that might help would be to move the fromIndex and toIndex
fields into AbstractList -- then fix every list operation to respect
those bounds rather than using 0 and array.length or whatever.  Then
sublist becomes trivial.


http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode199
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:199:
public void testSubList() {
My concern is that since there is no overriden version of
remove(Object), the superclass version won't respect fromIndex and
toIndex, so it will remove the element from the base list even though it
is not part of the sublist.

It's possible that this works fine -- I haven't tried to examine all the
superclass code -- but in any case I would like to see it as part of the
test case.

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode212
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:212:
assertEquals(testList, Arrays.asList(2, 6, 4));
OK

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-22 Thread rice 

LGTM but would like to see more tests if possible


http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode199
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:199:
public void testSubList() {
OK, I see that the implementation uses iterators for these operations,
which will respect the list bounds.  So I'm not as concerned although
more tests would still be a good thing.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-22 Thread jat 

On 2010/06/21 21:54:34, markovuksanovic wrote:

in javadoc is says



The semantics of the list returned by this method become undefined if

the

backing list (i.e., this list) is structurally modified in any way

other than

via the returned list. (Structural modifications are those that change

the size

of this list, or otherwise perturb it in such a fashion that

iterations in

progress may yield incorrect results.)



Does this mean that once a sublist view is defined, and the

underlaying list is

modified - is the afore mentioned sublist view, sort of, invalidated?

I would

say yes, but would like to hear other comments as well. I tried this

on sun jdk

and once I modified the underlaying list and afterwords tried to

access and/or

modify the sublist view I got exceptions thrown.


Yes, it means if you modify the underlying list in a way that would
invalidate iterators on it (such as adding an element), the behavior of
the sublist is undefined.  That means it could work properly, it might
throw exceptions, or it might totally destroy the underlying data
structure.  In general, the Sun JRE detects such conditions and throws
exceptions, but we don't do so in our JRE - the overhead would be rather
large.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-22 Thread markovuksanovic 

Thanks for the explanations. I'll add some tests and if necessary
override the remove method.


http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode199
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:199:
public void testSubList() {
I'll add some tests to verify the behavior.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-21 Thread markovuksanovic 

@jat, Any news about this?

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-21 Thread rice 

FYI


http://gwt-code-reviews.appspot.com/620802/diff/8001/9001
File user/super/com/google/gwt/emul/java/util/AbstractList.java (right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9001#newcode144
user/super/com/google/gwt/emul/java/util/AbstractList.java:144: }
I think you also need to implement remove(Object) so it only removes the
element if it lies in the wrapped range (see test code comments).

In general I think we need a bunch more tests before I would feel
confident that this implements the full List contract.

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode199
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:199:
public void testSubList() {
I think this test will fail (passes in JDK 1.6):

   ArrayListString wrappedList = new ArrayListString();
   wrappedList.add(A);
   wrappedList.add(B);

   ListString sub = wrappedList.subList(1, 1);
   sub.remove(A); // no A in range

   // original list is unchanged
   assertEquals(2, wrappedList.size());
   assertEquals(A, wrappedList.get(0));

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode212
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:212:
assertEquals(testList, Arrays.asList(2, 6, 4));
Shouldn't the '2' have been removed?

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode324
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:324: }
On 2010/06/19 20:50:09, markovuksanovic wrote:

Are this tests enough? At the moment nothing useful strikes me.


I would like to see a bunch of tests that exercise general properties of
lists (like size(), isEmpty(), contains, containsAll, etc.) and have
them called with both constructed lists like 'new ArrayList' as well as
sublists.  Then I would feel more confident that the sublists satisfy
the entire List contract.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-21 Thread markovuksanovic 

I have one question...

in javadoc is says

The semantics of the list returned by this method become undefined if
the backing list (i.e., this list) is structurally modified in any way
other than via the returned list. (Structural modifications are those
that change the size of this list, or otherwise perturb it in such a
fashion that iterations in progress may yield incorrect results.)

Does this mean that once a sublist view is defined, and the underlaying
list is modified - is the afore mentioned sublist view, sort of,
invalidated? I would say yes, but would like to hear other comments as
well. I tried this on sun jdk and once I modified the underlaying list
and afterwords tried to access and/or modify the sublist view I got
exceptions thrown.


http://gwt-code-reviews.appspot.com/620802/diff/8001/9001
File user/super/com/google/gwt/emul/java/util/AbstractList.java (right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9001#newcode144
user/super/com/google/gwt/emul/java/util/AbstractList.java:144: }
I'll look into this.

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode199
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:199:
public void testSubList() {
This tests were already written. Only a whitespace was inserted. I think
that's defined by check styles.

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode212
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:212:
assertEquals(testList, Arrays.asList(2, 6, 4));
Depends, if 2 is object then yes, if 2 is index then no.n BTW, I didn't
write this test either... but will look into refactoring.

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode324
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:324: }
Ok, I'll try to think of some more tests. Just to let you know, in this
file there's one more test that exercises quite some stuff with the
sublist. In my opinion is should be split into a few smaller tests.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: List.subList not fully compatibile with java.util.List interface. (issue620802)

2010-06-19 Thread markovuksanovic 

Here is a patch set with a minor fix, and a few more tests.


http://gwt-code-reviews.appspot.com/620802/diff/8001/9002
File user/test/com/google/gwt/emultest/java/util/ListTestBase.java
(right):

http://gwt-code-reviews.appspot.com/620802/diff/8001/9002#newcode324
user/test/com/google/gwt/emultest/java/util/ListTestBase.java:324: }
Are this tests enough? At the moment nothing useful strikes me.

http://gwt-code-reviews.appspot.com/620802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors