You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by wg...@apache.org on 2006/09/24 05:55:33 UTC
svn commit: r449347 - in /jakarta/velocity/engine/trunk:
src/java/org/apache/velocity/runtime/parser/node/ASTMethod.java
src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java
xdocs/changes.xml
Author: wglass
Date: Sat Sep 23 20:55:33 2006
New Revision: 449347
URL: http://svn.apache.org/viewvc?view=rev&rev=449347
Log:
Fix to introspection method caching. Thanks to Alexey Panchenko for the catch, fix, and test. VELOCITY-453.
Added:
jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java (with props)
Modified:
jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTMethod.java
jakarta/velocity/engine/trunk/xdocs/changes.xml
Modified: jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTMethod.java
URL: http://svn.apache.org/viewvc/jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTMethod.java?view=diff&rev=449347&r1=449346&r2=449347
==============================================================================
--- jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTMethod.java (original)
+++ jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTMethod.java Sat Sep 23 20:55:33 2006
@@ -18,6 +18,7 @@
import java.lang.reflect.InvocationTargetException;
+import org.apache.commons.lang.ArrayUtils;
import org.apache.velocity.app.event.EventHandlerUtil;
import org.apache.velocity.context.InternalContextAdapter;
import org.apache.velocity.exception.MethodInvocationException;
@@ -45,7 +46,7 @@
*/
public class ASTMethod extends SimpleNode
{
- private String methodName = "";
+ protected String methodName = "";
private int paramCount = 0;
/**
@@ -126,7 +127,7 @@
* change from visit to visit
*/
- Class[] paramClasses = new Class[paramCount];
+ final Class[] paramClasses = paramCount > 0 ? new Class[paramCount] : ArrayUtils.EMPTY_CLASS_ARRAY;
for (int j = 0; j < paramCount; j++)
{
@@ -325,23 +326,51 @@
this.params = params;
}
+ private final ASTMethod getASTMethod()
+ {
+ return ASTMethod.this;
+ }
+
/**
* @see java.lang.Object#equals(java.lang.Object)
*/
public boolean equals(Object o)
{
- return (o != null) &&
- (o instanceof MethodCacheKey) &&
- (this.hashCode() == o.hashCode());
+ if (o instanceof MethodCacheKey)
+ {
+ final MethodCacheKey other = (MethodCacheKey) o;
+ if (params.length == other.params.length && methodName.equals(other.getASTMethod().methodName))
+ {
+ for (int i = 0; i < params.length; ++i)
+ {
+ if (params[i] != other.params[i])
+ {
+ return false;
+ }
+ }
+ return true;
+ }
+ }
+ return false;
}
+
/**
* @see java.lang.Object#hashCode()
*/
- public int hashCode()
+ public int hashCode()
{
- return params.hashCode() * 37 + ASTMethod.this.hashCode();
+ int result = 0;
+ for (int i = 0; i < params.length; ++i)
+ {
+ final Class param = params[i];
+ if (param != null)
+ {
+ result ^= param.hashCode();
+ }
+ }
+ return result * 37 + methodName.hashCode();
}
}
-
+
}
Added: jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java
URL: http://svn.apache.org/viewvc/jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java?view=auto&rev=449347
==============================================================================
--- jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java (added)
+++ jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java Sat Sep 23 20:55:33 2006
@@ -0,0 +1,71 @@
+package org.apache.velocity.test;
+
+/*
+ * Copyright 2000-2004 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.
+ */
+
+import java.io.IOException;
+import java.io.StringWriter;
+
+import org.apache.velocity.VelocityContext;
+import org.apache.velocity.app.Velocity;
+import org.apache.velocity.exception.MethodInvocationException;
+import org.apache.velocity.exception.ParseErrorException;
+import org.apache.velocity.exception.ResourceNotFoundException;
+import org.apache.velocity.util.introspection.IntrospectionCacheData;
+
+import junit.framework.TestCase;
+
+/**
+ * Checks that arrays are cached correctly in the Introspector.
+ *
+ * @author <a href="Alexey Pachenko">alex+news@olmisoft.com</a>
+ * @version $Id$
+ */
+public class IntrospectionCacheDataTestCase extends TestCase
+{
+
+ private static class CacheHitCountingVelocityContext extends VelocityContext
+ {
+ public int cacheHit = 0;
+
+ public IntrospectionCacheData icacheGet(Object key)
+ {
+ final IntrospectionCacheData result = super.icacheGet(key);
+ if (result != null) {
+ ++cacheHit;
+ }
+ return result;
+ }
+
+ }
+
+ public void testCache() throws ParseErrorException, MethodInvocationException,
+ ResourceNotFoundException, IOException
+ {
+ CacheHitCountingVelocityContext context = new CacheHitCountingVelocityContext();
+ context.put("this", this);
+ StringWriter w = new StringWriter();
+ Velocity.evaluate(context, w, "test", "$this.exec('a')$this.exec('b')");
+ assertEquals("[a][b]", w.toString());
+ assertTrue(context.cacheHit > 0);
+ }
+
+ public String exec(String value)
+ {
+ return "[" + value + "]";
+ }
+
+}
Propchange: jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java
------------------------------------------------------------------------------
svn:eol-style = native
Propchange: jakarta/velocity/engine/trunk/src/test/org/apache/velocity/test/IntrospectionCacheDataTestCase.java
------------------------------------------------------------------------------
svn:keywords = Id Author Date Revision
Modified: jakarta/velocity/engine/trunk/xdocs/changes.xml
URL: http://svn.apache.org/viewvc/jakarta/velocity/engine/trunk/xdocs/changes.xml?view=diff&rev=449347&r1=449346&r2=449347
==============================================================================
--- jakarta/velocity/engine/trunk/xdocs/changes.xml (original)
+++ jakarta/velocity/engine/trunk/xdocs/changes.xml Sat Sep 23 20:55:33 2006
@@ -24,6 +24,10 @@
<body>
<release version="1.5-dev" date="in Subversion">
+ <action type="fix" dev="wglass" issue="VELOCITY-453" due-to="Alexey Panchenko">
+ Method caching now uses consistent keys.
+ </action>
+
<action type="fix" dev="wglass" issue="VELOCITY-459" due-to="Stephen Haberman">
Change the meaning of localscope for macros to allow access to references from
calling context.
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: svn commit: r449347 - in /jakarta/velocity/engine/trunk:
Posted by Henning Schmiedehausen <hp...@intermeta.de>.
Hi Alexey,
thanks a lot for your patch. I like it and we should apply it. I very
much appreciate that you understood my mail as peer review and not
critique per se.
[...]
> This is just waste of processor cycles.
> The Class does not override the equals().
Hm. It does not for the Sun JDK and I'm pretty sure that it will not be
overridden by any other JDK, but this is IMHO not guaranteed.
One of my "Java memes" is "thou shalt never compare two objects with ==
or !=". A class object is definitely an object. Yes, the default
implementation of equals() inside Object is '=='.
However, it is the job of the compiler to optimize that away, not of the
developer to second guess it. So for the sake of being anal, I'd like to
see equals() here. :-)
> And it never be overriden.
Sure? /me thinks of weird class loader things and runtime byte code
'improvements'... :-) But again, I'm splitting hairs 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.
>
> You want to compare the speed of
[... HCB vs. hand-rolled code removed ...]
Yes, because I wouldn't use the Reflection code but add the attributes
explicitly and again, I will not second guess the compiler. I'll try to
get some numbers tonight...
[...]
>> An unit test draft could look like this:
>
> ...
>
>> 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());
>> }
>
> I do not agree with this test. It is not required (and is not always
> possible) that different objects return different hashcodes.
Yes, you are right. That is a leftover and should not be there. As I
said, this was a draft. (I do know that the smallest legal
implementation of hashCode() is public int hashCode() { return 42; } ;-) )
Best regards
Henning
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re[2]: svn commit: r449347 - in /jakarta/velocity/engine/trunk:
Posted by Alexey Panchenko <al...@olmisoft.com>.
Henning P. Schmiedehausen wrote:
> (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).
I agree with part of the critiques, and disagree with the other part.
>>- private String methodName = "";
>>+ protected String methodName = "";
ok
> 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.
This is just waste of processor cycles.
The Class does not override the equals().
And it never be overriden.
> 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.
You want to compare the speed of
for (int i = 0; i < params.length; ++i) {
final Class param = params[i];
if (param != null) {
result ^= param.hashCode();
}
}
with the code in HashCodeBuilder:
public static int reflectionHashCode(
int initialNonZeroOddNumber,
int multiplierNonZeroOddNumber,
Object object,
boolean testTransients,
Class reflectUpToClass,
String[] excludeFields) {
if (object == null) {
throw new IllegalArgumentException("The object to build a hash code for must not be null");
}
HashCodeBuilder builder = new HashCodeBuilder(initialNonZeroOddNumber, multiplierNonZeroOddNumber);
Class clazz = object.getClass();
reflectionAppend(object, clazz, builder, testTransients, excludeFields);
while (clazz.getSuperclass() != null && clazz != reflectUpToClass) {
clazz = clazz.getSuperclass();
reflectionAppend(object, clazz, builder, testTransients, excludeFields);
}
return builder.toHashCode();
}
private static void reflectionAppend(
Object object,
Class clazz,
HashCodeBuilder builder,
boolean useTransients,
String[] excludeFields) {
Field[] fields = clazz.getDeclaredFields();
List excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.EMPTY_LIST;
AccessibleObject.setAccessible(fields, true);
for (int i = 0; i < fields.length; i++) {
Field f = fields[i];
if (!excludedFieldList.contains(f.getName())
&& (f.getName().indexOf('$') == -1)
&& (useTransients || !Modifier.isTransient(f.getModifiers()))
&& (!Modifier.isStatic(f.getModifiers()))) {
try {
builder.append(f.get(object));
} catch (IllegalAccessException e) {
//this can't happen. Would get a Security exception instead
//throw a runtime exception in case the impossible happens.
throw new InternalError("Unexpected IllegalAccessException");
}
}
}
}
and so on ?
> The hash method uses the ^= method to add the parameters. This is a
> really bad idea, because it detoriates the hash value.
ok, the hash function could be better.
> I know that "this is private code (if it is, why is MethodCacheKey
> package protected and not private?)"
This is the question to the original author of this class.
I just wanted to minimize changes in my patch.
> An unit test draft could look like this:
...
> 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());
> }
I do not agree with this test. It is not required (and is not always
possible) that different objects return different hashcodes.
--
Best regards,
Alexey mailto:alex+news@olmisoft.com
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re[2]: svn commit: r449347 - in /jakarta/velocity/engine/trunk:
Posted by Alexey Panchenko <al...@olmisoft.com>.
Will Glass-Husain wrote:
> 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.
I have added the following patch to VELOCITY-453
It is small patch, so it could be applied quickly.
The changes are:
- private String methodName
- MethodCacheKey is a static class
Index: ASTMethod.java
===================================================================
--- ASTMethod.java (revision 450716)
+++ ASTMethod.java (working copy)
@@ -46,7 +46,7 @@
*/
public class ASTMethod extends SimpleNode
{
- protected String methodName = "";
+ private String methodName = "";
private int paramCount = 0;
/**
@@ -143,7 +198,7 @@
* check the cache
*/
- MethodCacheKey mck = new MethodCacheKey(paramClasses);
+ MethodCacheKey mck = new MethodCacheKey(methodName, paramClasses);
IntrospectionCacheData icd = context.icacheGet( mck );
/*
@@ -317,20 +338,17 @@
* Internal class used as key for method cache. Combines
* ASTMethod fields with array of parameter classes.
*/
- class MethodCacheKey
+ private static class MethodCacheKey
{
- Class[] params;
+ private final String methodName;
+ private final Class[] params;
- MethodCacheKey(Class[] params)
+ private MethodCacheKey(String methodName, Class[] params)
{
+ this.methodName = methodName;
this.params = params;
}
- private final ASTMethod getASTMethod()
- {
- return ASTMethod.this;
- }
-
/**
* @see java.lang.Object#equals(java.lang.Object)
*/
@@ -339,7 +357,7 @@
if (o instanceof MethodCacheKey)
{
final MethodCacheKey other = (MethodCacheKey) o;
- if (params.length == other.params.length && methodName.equals(other.getASTMethod().methodName))
+ if (params.length == other.params.length && methodName.equals(other.methodName))
{
for (int i = 0; i < params.length; ++i)
{
--
Best regards,
Alexey mailto:alex+news@olmisoft.com
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: svn commit: r449347 - in /jakarta/velocity/engine/trunk:
Posted by Henning Schmiedehausen <hp...@intermeta.de>.
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 <hp...@intermeta.de> wrote:
> > wglass@apache.org 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:wglass@apache.org">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
> > hps@intermeta.de +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: velocity-dev-unsubscribe@jakarta.apache.org
> > For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
> >
> >
>
>
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
hps@intermeta.de +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: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: svn commit: r449347 - in /jakarta/velocity/engine/trunk:
Posted by Will Glass-Husain <wg...@forio.com>.
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 <hp...@intermeta.de> wrote:
> wglass@apache.org 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:wglass@apache.org">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
> hps@intermeta.de +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: velocity-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: svn commit: r449347 - in /jakarta/velocity/engine/trunk:
Posted by "Henning P. Schmiedehausen" <hp...@intermeta.de>.
wglass@apache.org 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:wglass@apache.org">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
hps@intermeta.de +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: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org