Reviewers: amit, scottb, Message: LGTM
I am not sure I agree with trying to match unspecified behavior (see comments) when there is a size/speed cost for doing so, but I am ok with it. http://gwt-code-reviews.appspot.com/33819/diff/1/3 File user/super/com/google/gwt/emul/java/util/TreeMap.java (right): http://gwt-code-reviews.appspot.com/33819/diff/1/3#newcode966 Line 966: * create a new node "newNode" to replace the "found" node. From my reading of the JRE docs, it is unspecified that a previous Entry object returned from entrySet() can be assumed valid after it is deleted. So, I am not sure that it is appropriate to match the JRE's behavior when it is unspecified and there is some expense in doing so. The Entry object returned is an internal object, and I think code that relies on it remaining valid after deleting the entry is asking for trouble and should instead save the key/value if they need it. http://gwt-code-reviews.appspot.com/33819/diff/1/3#newcode993 Line 993: private void replaceNode(Node<K, V> head, Node<K, V> node, Node<K, V> newNode) { I dislike adding this extra pass, and I think it should be possible to use the rebalancing process to push the deleted node down to a leaf and then just detach it. However, that would likely complicate the rotation logic significantly. Suggest adding a TODO to investigate that. Please review this at http://gwt-code-reviews.appspot.com/33819 Affected files: M user/super/com/google/gwt/emul/java/util/TreeMap.java M user/test/com/google/gwt/emultest/java/util/TreeMapStringStringTest.java --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
