+1 Incidentally, I tested your previous patch and it looks good to me. I also tried running the commons-collections tests - thinking that these might be a good way to achieve better coverage - and they also pass except for one unrelated failure (HARMONY-5788).
I've not had chance to look at the performance implications yet. -Mark. [0] http://commons.apache.org/collections/ In message <[EMAIL PROTECTED]>, "Aleksey Shipilev" writes: > > Thanks, Alexey! > > I see the plan is following: > 1. Sweep the HashMap implementation and make the source beatuful: add > necessary comments, re-layout class members. > 2. Test-commit-test sweeped HashMap implementation and see there are > no breakages. > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn > capabilities) > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode -> > identityHashCode, equals -> == and stuff) > 5. Test-commit-test new IdentityHashMap. > > Then we could think about generalization of IdentityHashMap and HashMap code. > > Nathan, Mark, Alexey, do I summarize correct? > > Thanks, > Aleksey. > > On Wed, Apr 23, 2008 at 4:43 PM, Alexey Petrenko > <[EMAIL PROTECTED]> wrote: > > Aleksey, > > > > I would support Mark here. Clear patch is very important because it > > makes our lives much easier and we are not required to skip 70K of > > irrelevant text to see 2 relevant lines :) > > So I think that your idea is good but should be better delivered :) > > > > Take a look at "Good Issue Resolution Guideline" for example. > > http://harmony.apache.org/issue_resolution_guideline.html > > > > SY, Alexey > > > > 2008/4/23, Mark Hindess <[EMAIL PROTECTED]>: > > > > > > > > > > On 23 April 2008 at 0:36, "Aleksey Shipilev" <[EMAIL PROTECTED] > > > > > wrote: > > > > Hi Endre, > > > > > > > > On Tue, Apr 22, 2008 at 11:30 PM, Endre St=F8lsvik <[EMAIL PROTECTED] > > wro= > > > > te: > > > > > Aleksey Shipilev wrote: > > > > > > The reason behind all that changes is that entire IdentityHashMap > > > > > > implementation was thrown away and replaced by HashMap > > > > > > > > > > Isn't it possible to actually record this fact using SVN, by > > > > > deleting the file, and then adding it again (or svn copy it from > > > > > HashMap) - so that it doesn't look like a *change*, but more what it > > > > > actually is: a remove, and then an add (actually, a copy)? > > > > > > > > Unfortunately, that's not usable, you might play around to see why. If > > > > you find a solution, please let me know :) > > > > > > Fortunately, it is not compulsory to create patches with "svn diff". > > > I've just done: > > > > > > 1) apply your patch to a fresh checkout > > > 2) cp modules/luni/src/main/java/java/util/HashMap.java \ > > > modules/luni/src/main/java/java/util/IdentityHashMap.java.orig > > > 3) diff -u modules/luni/src/main/java/java/util/IdentityHashMap.java.ori > g \ > > > modules/luni/src/main/java/java/util/IdentityHashMap.java > > > > > > The resulting patch would be much more suitable for attachment to a JIRA > . > > > > > > I'd still fix a few things about HashMap.java first though. > > > -Mark. > > > > > > > > > > > >
