[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Fix for Hashtable contains blowing up the stack

From: Dalibor Topic
Subject: Re: [PATCH] Fix for Hashtable contains blowing up the stack
Date: Fri, 28 Nov 2003 17:39:39 +0100
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312

Hi Mark, hallo Jeroen,

Mark Wielaard wrote:

On Sat, 2003-11-22 at 13:06, Jeroen Frijters wrote:

The fix is actually incorrect. containsValue() should call contains()
because containsValue() is a new method (since 1.2) and contains()
exists since 1.0. Older code may have overridden contains() and this
should work with newer code that calls containsValue().

Urgh. Yes, you are right. Wrote an additional test case for this: (it currently fails).

Thanks for pointing that mistake out. I've attached an updated version of the patch.

2003-11-28  Dalibor Topic <address@hidden>

        Reported by: Jim Pick <address@hidden>

        * libraries/javalib/java/util/
        (internalcontainsValue): New method.
        (contains) Delegate to internalContainsValue.

        Reported by: Mark Wielaard  <address@hidden>

        * libraries/javalib/java/util/
        (contains): Improved comment.

        Reported by: Jeroen Frijters  <address@hidden>

        * libraries/javalib/java/util/
        (containsValue): Delegate to contains(Object) to make sure older
        code overwriting it continues to work.

I'm not sure how to deal with mutliple people's bug reports being fixed in this patch in the ChangeLog entry, so I'd appreciate a hint from the ChangeLog police.

As I've argued before, in cases such as these (multiple virtual methods
that do the same thing), we must use the same delegation as the Sun
implementation to be compatible.

Problem is that this is not always easy to know.
So we depend on bug reports and then have to write a Mauve test if we
think that following such behavior is beneficial. But the "newer calls
older" is a guideline that will mostly work (I hope).

Yes, that makes the most sense to me, too. Stuart Ballard started working on a 'override compatibility' test suite for collection classes. See [1] for details.

dalibor topic

--- /var/tmp/PROJECTS/classpath/java/util/        Wed Nov 12 
21:56:20 2003
+++ kaffe/libraries/javalib/java/util/    Fri Nov 28 17:11:51 2003
@@ -333,7 +333,11 @@
   public synchronized boolean contains(Object value)
-    return containsValue(value);
+    /* delegate to non-overridable worker method 
+     * to avoid blowing up the stack, when called 
+     * from overridden contains[Value]() method.
+     */
+    return internalContainsValue(value);
@@ -349,6 +353,25 @@
    * @since 1.2
   public boolean containsValue(Object value)
+  {
+    /* delegate to older method to make sure code overwriting it 
+     * continues to work.
+     */
+    return contains(value);
+  }
+  /**
+   * Returns true if this Hashtable contains a value <code>o</code>, such that
+   * <code>o.equals(value)</code>. This is an internal worker method
+   * called by <code>contains()</code> and <code>containsValue()</code>.
+   *
+   * @param value the value to search for in this Hashtable
+   * @return true if at least one key maps to the value
+   * @see #contains(Object)
+   * @see #containsKey(Object)
+   * @throws NullPointerException if <code>value</code> is null
+   */
+  private boolean internalContainsValue(Object value)
     for (int i = buckets.length - 1; i >= 0; i--)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]