You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by he...@apache.org on 2006/09/16 12:31:11 UTC

svn commit: r446846 - in /jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node: ASTEQNode.java ASTNENode.java

Author: henning
Date: Sat Sep 16 03:31:11 2006
New Revision: 446846

URL: http://svn.apache.org/viewvc?view=rev&rev=446846
Log:
A hard to see bug. At the location of the (left == null ? 0 : 1),
left never could be null, because this has been tested earlier.

Actually meant was left.toString() == null, which also should not
happen (the contract of toString() normally forbids it), but nevertheless
fix the problem. Suggested by FindBugs.

Modified:
    jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java
    jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java

Modified: jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java
URL: http://svn.apache.org/viewvc/jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java?view=diff&rev=446846&r1=446845&r2=446846
==============================================================================
--- jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java (original)
+++ jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java Sat Sep 16 03:31:11 2006
@@ -130,10 +130,11 @@
              */
             if ((left.toString() == null) || (right.toString() == null))  
             {
-                log.error((left.toString() == null ? "Left" : "Right") 
+        	boolean culprit =  (left.toString() == null);
+                log.error((culprit ? "Left" : "Right") 
                         + " string side "
                         + "String representation ("
-                        + jjtGetChild( (left == null? 0 : 1) ).literal()
+                        + jjtGetChild((culprit ? 0 : 1) ).literal()
                         + ") of '!=' operation has null value."
                         + " Operation not possible. "
                         + context.getCurrentTemplateName() + " [line " + getLine() 

Modified: jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java
URL: http://svn.apache.org/viewvc/jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java?view=diff&rev=446846&r1=446845&r2=446846
==============================================================================
--- jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java (original)
+++ jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java Sat Sep 16 03:31:11 2006
@@ -112,10 +112,11 @@
              */
             if ((left.toString() == null) || (right.toString() == null))  
             {
-                log.error((left.toString() == null ? "Left" : "Right")
+        	boolean culprit =  (left.toString() == null);
+                log.error((culprit ? "Left" : "Right") 
                         + " string side "
                         + "String representation ("
-                        + jjtGetChild( (left == null? 0 : 1) ).literal()
+                        + jjtGetChild((culprit ? 0 : 1) ).literal()
                         + ") of '!=' operation has null value."
                         + " Operation not possible. "
                         + context.getCurrentTemplateName() + " [line " + getLine() 



---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org


Re: svn commit: r446846 - in /jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node: ASTEQNode.java ASTNENode.java

Posted by "Henning P. Schmiedehausen" <hp...@intermeta.de>.
"Nathan Bubna" <nb...@gmail.com> writes:

>Just for the record, i agree with fixing this problem.  I feel we
>should always consider the possibility of toString() returning null
>when we are working in the Velocity world.  While the contract of
>toString() does normally forbid it, Velocity's high dependence on
>toString() makes it unwise to trust programmers to know this, regard
>it and successfully follow it.

I completely agree. Did I BTW mention that I absolutely adore
FindBugs? :-) That bugger is not only correct 99 out of a 100 times,
it also finds all that nitty, gritty stuff that one tend to look over.

And while the Eclipse plugin has its edges, I really like the little
bugs popping up in my code... :-)

	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


Re: svn commit: r446846 - in /jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node: ASTEQNode.java ASTNENode.java

Posted by Nathan Bubna <nb...@gmail.com>.
On 9/16/06, henning@apache.org <he...@apache.org> wrote:
> Author: henning
> Date: Sat Sep 16 03:31:11 2006
> New Revision: 446846
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=446846
> Log:
> A hard to see bug. At the location of the (left == null ? 0 : 1),
> left never could be null, because this has been tested earlier.
>
> Actually meant was left.toString() == null, which also should not
> happen (the contract of toString() normally forbids it), but nevertheless
> fix the problem. Suggested by FindBugs.

Just for the record, i agree with fixing this problem.  I feel we
should always consider the possibility of toString() returning null
when we are working in the Velocity world.  While the contract of
toString() does normally forbid it, Velocity's high dependence on
toString() makes it unwise to trust programmers to know this, regard
it and successfully follow it.

---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org