I did some analysis on what API methods access others between the RI and Harmony (by subclassing ArrayList and adding some trace) to answer a couple of the review points. The differences I found were: - RI contains(Object) calls indexOf(Object), so we could also do this to reduce the code duplication - RI add(Object), add(int, Object), addAll(Collection), addAll(int, Collection) use ensureCapacity(int) - RI remove(Object) doesn't reference indexOf(Object) and remove(int) - though I don't think this would save anything if we changed it
Cath On Tue, Aug 3, 2010 at 11:45 PM, Mark Hindess <mark.hind...@googlemail.com>wrote: > > In message <4c56e4f8.2040...@googlemail.com>, Oliver Deakin writes: > > > > Hi all, > > > > A little while ago Mark suggested (not sure if it was in person or > > on the list) that it would be a good idea to take a look at our > > Collections classes and do a kind of code review, thinking about > > improvements and possible optimisations. > > > > To that end Mark Hindess, Cath Hope and myself sat down and took a > > close look at ArrayList earlier today. We got up to the growAtEnd() > > method, annotating the code with questions and comments as we went. > > I've opened HARMONY-6607 [1] and attached the diff with our comments > > in so everyone can see what we've come up with so far. > > > > If you have any further comments or ideas for the code (including > > patches for improvements) please add to the discussion here - it would > > be great to have some extra input! > > We had another chat today and got a bit further. I've updated the > review patch on JIRA. > > I think there was general consensus that we should move all the > exceptions to the beginning of methods. (And in the case of removeRange > fix the exception to be meaningful.) I'll probably look at doing this > shortly unless someone beats me too it as it was really annoying when > reading the code. > > We should also consider tracking firstIndex and size rather than firstIndex > and lastIndex because: > > * Nearly every method uses size - most calculate it but many of them > "forget" to use it in all cases. > > * Fewer methods use lastIndex. > > * size() is the most commonly called method making it simpler would be > a useful side-effect > > * the resulting serialization code might be simpler > > I'm still puzzled as to whether the fact that the call to growForInsert > in addAll always grows by the size of the entire collection (rather than > size - free space) is intentional or a bug. The increment calculation > should mean we always have sufficient free space even if we only ask for > the smaller (strictly necessary) amount. So I suspect it is a bug? > > Regards, > Mark. > > > [1] https://issues.apache.org/jira/browse/HARMONY-6607 >
