Hi,
I fully agree with you here and it really is a good thing that this bug
got found and fixed. Yes, really nice job finding that, Alexey. The
whole business around hashCode() and equals() is very often overlooked
and was apparently so by us. It is good that there are contributors to
find and fix that kind of problems.
Also thanks Will, for working with Alexey here; a problem that we had in
the past is that good patches and code was sitting in the bug tracker
for far too long, something that has been criticized by other before.
Good to see that patches get applied in a somewhat timely fashion.
Getting Velocity to move at a somewhat more than glacial fashion (which
was a bit of irony for a project called "Velocity") is really good. We
do need contributors like Alexey for that. Once again, thanks to you
both for teaming up here.
Best regards
Henning
On Wed, 2006-09-27 at 20:22 -0700, Will Glass-Husain wrote:
> Wow. That's a lot of comments, all good ones. I'd hate to see you
> sad, Henning, your cheery disposition lights up this project. :-)
>
> One quick point. I want to re-iterate what I said in the JIRA note.
> This is a bug that's been in Velocity a long time, in a fundamental
> part of the caching routine. Nice job, Alexey, on finding this,
> fixing it, and jumping through all the hoops to get the patch in.
> Hope to see more.
>
> I'll take responsibility to work with Alexey in cleaning this up.
> Though to be fair, you are holding the patch to a higher standard than
> the original. Of course the original had a bug.
>
> And the typo in the copyright date is my fault. Legal issues are
> really the responsibility of the committers :-)
>
> WILL
>
>
> On 9/27/06, Henning P. Schmiedehausen <[EMAIL PROTECTED]> wrote:
> > [EMAIL PROTECTED] writes:
> >
> > (This mail might sound a bit hash. It is not intended as such, but I
> > want to make sure that we do get peer review with code going in, even
> > if it is "just a small fix". So I intend to use this as an example
> > case. Sorry 'bout that. This message is intended to criticize the
> > code, not you or Alexey).
> >
> > >- private String methodName = "";
> > >+ protected String methodName = "";
> >
> > [...]
> >
> > >+ if (params.length == other.params.length &&
> > >methodName.equals(other.getASTMethod().methodName))
> >
> > I'm -0.5 on that patch. That coding style is not good (private is
> > there for a reason), the change to protected is not even needed,
> > because if you are an object of class "Foo", you can look at the
> > private fields of every other Foo, too:
> >
> > public class Foo {
> > private final int value;
> >
> > public Foo(final int value) { this.value = value; }
> >
> > public void report(final Foo foo) {
> > System.out.println(foo.value);
> > }
> > }
> >
> >
> > public class Bar {
> > public static void main(String[] args) {
> > Foo foo1 = new Foo(23);
> > Foo foo2 = new Foo(42);
> > foo2.report(foo1);
> > }
> > }
> >
> > Run "Bar" and you will get "23" as the result. foo2 is allowed to look
> > at the private value field of foo1.
> >
> > If we need to look at member fields of any class, we add getters and
> > setters, not change the visibility of the member fields. It is the job
> > of the Java compiler (which does a very good job here) to optimize
> > that.
> >
> > I don't like the fact that there is object comparing (the Class
> > elements of params should be compared using equals, not != ) in the
> > code.
> >
> > The code doesn't adhere to our indent rules and the year date in the
> > copyright notice is wrong (should be 2006). I freely admit that I'm
> > picking nits here, mainly because I don't like the implementation of
> > the methods and don't believe in the handwaving that "this is faster
> > than using EqualsBuilder and HashCodeBuilder because of Object
> > creation". I haven't seen any numbers to prove or disprove that claim.
> >
> > The hash method uses the ^= method to add the parameters. This is a
> > really bad idea, because it detoriates the hash value. Consider two
> > arrays:
> >
> > Class [] e1 = new Class [] { String.class, String.class };
> > Class [] e2 = new Class [] { Object.class, Object.class };
> >
> > int result = 0;
> > for (int i = 0; i < params.length; ++i)
> > {
> > final Class param = params[i];
> > if (param != null)
> > {
> > result ^= param.hashCode();
> > }
> > }
> >
> > That loop returns the same value for both arrays: 0 (The reason why is
> > left for the reader as an exercise :-) ).
> >
> > Yes, it *is* hard to write a good hash function. That is why there is
> > so much literature about it.
> > E.g. http://java.sun.com/developer/Books/effectivejava/Chapter3.pdf
> >
> > (In "Hard Core Java", the methods in the Jakarta commons are
> > explicitly mentioned as examples for a good hash code function. There
> > is a reason for this).
> >
> > I know that "this is private code (if it is, why is MethodCacheKey
> > package protected and not private?)" and "it can never be called with
> > params == null". However, if you do, you get an NPE. If it is possible
> > to make that code robust, why not do it?
> >
> > An unit test draft could look like this:
> >
> > --- cut ---
> > package org.apache.velocity.runtime.parser.node;
> >
> > import junit.framework.Test;
> > import junit.framework.TestSuite;
> >
> > import org.apache.commons.lang.ArrayUtils;
> > import org.apache.velocity.runtime.parser.node.ASTMethod.MethodCacheKey;
> > import org.apache.velocity.test.BaseTestCase;
> >
> > /*
> > * Copyright 2006 The Apache Software Foundation.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License")
> > * you may not use this file except in compliance with the License.
> > * You may obtain a copy of the License at
> > *
> > * http://www.apache.org/licenses/LICENSE-2.0
> > *
> > * Unless required by applicable law or agreed to in writing, software
> > * distributed under the License is distributed on an "AS IS" BASIS,
> > * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > * See the License for the specific language governing permissions and
> > * limitations under the License.
> > */
> >
> > /**
> > * Test ASTMethod equals() and hashCode()
> > *
> > * @author <a href="mailto:[EMAIL PROTECTED]">Will Glass-Husain</a>
> > * @version $Id: ResourceCachingTestCase.java 447969 2006-09-19 21:00:14Z
> > henning $
> > */
> > public class MethodKeyTestCase extends BaseTestCase
> > {
> > /**
> > * Default constructor.
> > */
> > public MethodKeyTestCase(String name)
> > {
> > super(name);
> > }
> >
> > public void setUp()
> > throws Exception
> > {
> >
> > }
> >
> > public static Test suite()
> > {
> > return new TestSuite(MethodKeyTestCase.class);
> > }
> >
> > public void testEquals()
> > {
> > Class [] elements = new Class [] { Object.class };
> > ASTMethod m1 = new ASTMethod(23);
> > MethodCacheKey mck = m1.new MethodCacheKey(elements);
> >
> > assertTrue(mck.equals(mck));
> > assertTrue(!mck.equals(null));
> > assertTrue(!mck.equals((MethodCacheKey) null));
> > }
> >
> > public void testEqualsPathological()
> > {
> > Class [] elements = new Class [] {};
> > ASTMethod m1 = new ASTMethod(23);
> > MethodCacheKey mck = m1.new MethodCacheKey(elements);
> >
> > assertTrue(mck.equals(mck));
> > assertTrue(!mck.equals(null));
> > assertTrue(!mck.equals((MethodCacheKey) null));
> > }
> >
> > public void testEqualsPathological2()
> > {
> > Class [] elements = new Class [] {null};
> > ASTMethod m1 = new ASTMethod(23);
> > MethodCacheKey mck = m1.new MethodCacheKey(elements);
> >
> > assertTrue(mck.equals(mck));
> > assertTrue(!mck.equals(null));
> > assertTrue(!mck.equals((MethodCacheKey) null));
> > }
> >
> > public void testEqualsBad()
> > {
> > Class [] elements = ArrayUtils.EMPTY_CLASS_ARRAY;
> > ASTMethod m1 = new ASTMethod(23);
> > MethodCacheKey mck = m1.new MethodCacheKey(elements);
> >
> > assertTrue(mck.equals(mck));
> > assertTrue(!mck.equals(null));
> > assertTrue(!mck.equals((MethodCacheKey) null));
> > }
> >
> > public void testEqualsWorse()
> > {
> > Class [] elements = null;
> > ASTMethod m1 = new ASTMethod(23);
> > MethodCacheKey mck = m1.new MethodCacheKey(elements);
> >
> > assertTrue(mck.equals(mck));
> > assertTrue(!mck.equals(null));
> > assertTrue(!mck.equals((MethodCacheKey) null));
> > }
> >
> > public void testHashCode()
> > {
> > Class [] e1 = new Class [] { String.class, String.class };
> > Class [] e2 = new Class [] { Object.class, Object.class };
> >
> > ASTMethod m1 = new ASTMethod(23);
> > MethodCacheKey mck1 = m1.new MethodCacheKey(e1);
> > MethodCacheKey mck2 = m1.new MethodCacheKey(e2);
> >
> > assertTrue(mck1.hashCode() != mck2.hashCode());
> > }
> > }
> >
> >
> > --- cut ---
> >
> > I'd like the code to pass all of those tests. Yes, there is a number
> > of pathologic tests.
> >
> > To make a medium length story short: That code would not pass my
> > personal QA. I'd like to get it reviewed and fixed. I will not insist
> > on it (that's why it is -0.5 and not -1 but it will make me sad if the
> > code stays as it is).
> >
> >
> > Best regards
> > Henning
> >
> > --
> > Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
> > [EMAIL PROTECTED] +49 9131 50 654 0 http://www.intermeta.de/
> >
> > RedHat Certified Engineer -- Jakarta Turbine Development -- hero for hire
> > Linux, Java, perl, Solaris -- Consulting, Training, Development
> >
> > Social behaviour: Bavarians can be extremely egalitarian and folksy.
> > -- http://en.wikipedia.org/wiki/Bavaria
> > Most Franconians do not like to be called Bavarians.
> > --
> > http://en.wikipedia.org/wiki/Franconia
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > For additional commands, e-mail: [EMAIL PROTECTED]
> >
> >
>
>
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
[EMAIL PROTECTED] +49 9131 50 654 0 http://www.intermeta.de/
RedHat Certified Engineer -- Jakarta Turbine Development
Linux, Java, perl, Solaris -- Consulting, Training, Engineering
"For a successful technology, reality must take precedence over
public relations for Nature cannot be fooled" - Richard P. Feynman
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
