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

[Dalibor Topic]

Hi Mark, hallo Jeroen,

Mark Wielaard wrote:
> Hi
>
> 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:
> gnu.testlet.java.util.Hashtable.ContainsHash (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/Hashtable.java
        (internalcontainsValue): New method.
        (contains) Delegate to internalContainsValue.

        Reported by: Mark Wielaard  <address@hidden>

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

        Reported by: Jeroen Frijters  <address@hidden>

        * libraries/javalib/java/util/Hashtable.java
        (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.

cheers,
dalibor topic

[1] http://www.kaffe.org/pipermail/kaffe/2003-May/042181.html

--- /var/tmp/PROJECTS/classpath/java/util/Hashtable.java        Wed Nov 12 
21:56:20 2003
+++ kaffe/libraries/javalib/java/util/Hashtable.java    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--)
       {