Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: None
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other

      Description

      Hi,

      finally we got it together - the infamous "decimal number patch" evolved
      and here is the (hopefully) final patch to enable decimal numbers (and
      byte, short, long and BigInteger btw.) in velocity. This patch contains:

      • Decimal literals (by Will Glass-Husain)
        Now you can type things like #set ($foo = 3.14)
      • All arithmetic operations (+ - / *) now work with all Number-types
        available. Modulo (%) works with all except BigDecimal (there is no
        such method
      • All comparators (< > == != <= >=) work with all Number-types
      • A new type called TemplateNumber (o.a.v.util.TemplateNumber) is
        available which is treated as a Number in arithmetic operations and
        comparisons. (by Will Glass-Husain)
        I am not sure about this one though. But Will had good reasons to
        implement it - I think I just need some kind of "refreshing" on that

      The patch is against the current head (Geir, you gave me a hard time
      during the last days, since you changed the Parser.jjt quite often, so
      that I had to incorporate the changes quite some times

      Additionally some implementation details:
      -----------------------------------------

      All aritmetic operations and comparisons are delegated to a new class
      (o.a.v.runtime.parser.MathUtils). All operations are done using
      primitives (as far as possible) and NOT BigInteger / BigDecimal like the
      first proposal did. I added overflow checking all along the way (where
      possible). In addition to that I removed the "paradigm of the smallest
      type" where add (new Integer(1), new Integer(2)) would result in a
      Byte(3). Now types are preserved as far as possible (overflow, mixing of
      types). You find a lot of documentation about that in the javadoc
      (MathUtils.java). Finally, overall, there should be no performance drawback.
      There are also some new testcases and existing ones are updated (mostly
      template based like math.vm).

      So we (I hope Will doesn't mind me talking in his name) think this patch
      will increase the usability of velocity a lot and hope that the others
      like it too.

      Open for discussion...

      Regards
      Peter

      1. ASF.LICENSE.NOT.GRANTED--arithmetic.patch
        196 kB
        Peter Romianowski
      2. ASF.LICENSE.NOT.GRANTED--arithmetic.patch
        247 kB
        Peter Romianowski
      3. ASF.LICENSE.NOT.GRANTED--number-patch2.patch
        172 kB
        Will Glass-Husain

        Activity

        Hide
        Peter Romianowski added a comment -

        Created an attachment (id=9780)
        Patch to current head

        Show
        Peter Romianowski added a comment - Created an attachment (id=9780) Patch to current head
        Hide
        Geir Magnusson Jr added a comment -

        Any chance you can include the new files too? See the top of the patch...

        Show
        Geir Magnusson Jr added a comment - Any chance you can include the new files too? See the top of the patch...
        Hide
        Will Glass-Husain added a comment -
            • Bug 24792 has been marked as a duplicate of this bug. ***
        Show
        Will Glass-Husain added a comment - Bug 24792 has been marked as a duplicate of this bug. ***
        Hide
        Peter Romianowski added a comment -

        Created an attachment (id=9789)
        Corrected patch. First one was missing some files.

        Show
        Peter Romianowski added a comment - Created an attachment (id=9789) Corrected patch. First one was missing some files.
        Hide
        Will Glass-Husain added a comment -

        Found an error in the patch. Decimals can't be used as arguments to
        velocitymacros. The fix is simple. As I suspect the diff will be eventually
        regenerated, I just give the fix by hand.

        file: VMProxyArg.java
        line: 490

        old:
        case ParserTreeConstants.JJTFLOATINGPOINTLITERAL :

        { constant = true; staticObject = new Integer(callerReference); break; }

        new:
        case ParserTreeConstants.JJTFLOATINGPOINTLITERAL :

        { constant = true; staticObject = new Double(callerReference); break; }
        Show
        Will Glass-Husain added a comment - Found an error in the patch. Decimals can't be used as arguments to velocitymacros. The fix is simple. As I suspect the diff will be eventually regenerated, I just give the fix by hand. file: VMProxyArg.java line: 490 old: case ParserTreeConstants.JJTFLOATINGPOINTLITERAL : { constant = true; staticObject = new Integer(callerReference); break; } new: case ParserTreeConstants.JJTFLOATINGPOINTLITERAL : { constant = true; staticObject = new Double(callerReference); break; }
        Hide
        Will Glass-Husain added a comment -

        Found another error in the patch. When multiplying two integers, if the
        second integer is zero an exception is thrown.

        03-06 15:10:29 ERROR - Internal Error: java.lang.ArithmeticException: / by zero
        03-06 15:10:29 ERROR - java.lang.ArithmeticException: / by zero
        at org.apache.velocity.runtime.parser.node.MathUtils.multiply
        (MathUtils.java:331)
        at org.apache.velocity.runtime.parser.node.ASTMulNode.value
        (ASTMulNode.java:149)

        This odd result is caused by an overflow check. The following edit will fix
        the problem.

        MathUtils.java
        Line 331

        old code:
        // Overflow detection
        if (result / l2 != l1)

        { return toBigInteger( op1).multiply( toBigInteger( op2)); }

        new code:
        // Overflow detection
        if ((l2 != 0) && (result / l2 != l1)) { return toBigInteger( op1).multiply( toBigInteger( op2)); }
        Show
        Will Glass-Husain added a comment - Found another error in the patch. When multiplying two integers, if the second integer is zero an exception is thrown. 03-06 15:10:29 ERROR - Internal Error: java.lang.ArithmeticException: / by zero 03-06 15:10:29 ERROR - java.lang.ArithmeticException: / by zero at org.apache.velocity.runtime.parser.node.MathUtils.multiply (MathUtils.java:331) at org.apache.velocity.runtime.parser.node.ASTMulNode.value (ASTMulNode.java:149) This odd result is caused by an overflow check. The following edit will fix the problem. MathUtils.java Line 331 old code: // Overflow detection if (result / l2 != l1) { return toBigInteger( op1).multiply( toBigInteger( op2)); } new code: // Overflow detection if ((l2 != 0) && (result / l2 != l1)) { return toBigInteger( op1).multiply( toBigInteger( op2)); }
        Hide
        Will Glass-Husain added a comment -
            • Bug 31333 has been marked as a duplicate of this bug. ***
        Show
        Will Glass-Husain added a comment - Bug 31333 has been marked as a duplicate of this bug. ***
        Hide
        Will Glass-Husain added a comment -

        Not sure why this bug was marked today as 'fixed'. It's clearly not. (no
        patches applied to CVS.) Please post an explanation, or leave it open.

        Show
        Will Glass-Husain added a comment - Not sure why this bug was marked today as 'fixed'. It's clearly not. (no patches applied to CVS.) Please post an explanation, or leave it open.
        Hide
        Will Glass-Husain added a comment -

        Daniel Rall has asked whether this patch has any performance implications.
        Suggest we do some simple tests, then commit if difference is negligable.

        Show
        Will Glass-Husain added a comment - Daniel Rall has asked whether this patch has any performance implications. Suggest we do some simple tests, then commit if difference is negligable.
        Hide
        Shinobu Kawai added a comment -
            • Bug 33103 has been marked as a duplicate of this bug. ***
        Show
        Shinobu Kawai added a comment - Bug 33103 has been marked as a duplicate of this bug. ***
        Hide
        Will Glass-Husain added a comment -
            • Bug 14160 has been marked as a duplicate of this bug. ***
        Show
        Will Glass-Husain added a comment - Bug 14160 has been marked as a duplicate of this bug. ***
        Hide
        Will Glass-Husain added a comment -

        Re: performance testing

        I wrote a simple integer-math heavy template, and merged it 1000 times. I did
        this three times and averaged the time. The results were interesting.

        Velocity 1.4:
        with caching: 488 ms
        no caching: 23944 ms

        SVN head (as of 1-23-2004) + number patch:
        with caching: 604ms
        no caching: 24198

        The results were also the same for Velocity 1.4 and SVN head (without the
        number patch).

        Interestingly, I also did this with an empty template. The results were
        almost the same in each case.

        Conclusion - without caching there is little difference. With caching, there
        is a slowdown for parsing templates. The content of the template doesn't seem
        to matter.

        Let's discuss on the dev list.

        Show
        Will Glass-Husain added a comment - Re: performance testing I wrote a simple integer-math heavy template, and merged it 1000 times. I did this three times and averaged the time. The results were interesting. Velocity 1.4: with caching: 488 ms no caching: 23944 ms SVN head (as of 1-23-2004) + number patch: with caching: 604ms no caching: 24198 The results were also the same for Velocity 1.4 and SVN head (without the number patch). Interestingly, I also did this with an empty template. The results were almost the same in each case. Conclusion - without caching there is little difference. With caching, there is a slowdown for parsing templates. The content of the template doesn't seem to matter. Let's discuss on the dev list.
        Hide
        Will Glass-Husain added a comment -

        Well, I ran the test suite again. To my embarassment, there appears to be
        almost no difference between Velocity with the decimal patch and without. It
        appears I took the version of Velocity previously compiled in the 1.4 release
        and compared it with one compiled on my machine. When I ran the latest source
        (compiled in the same manner) with and without the decimal patch, the results
        were identical. 10000 merges of an integer math heavy file took an average of
        1532 ms (without the patch) and 1537 ms (with the patch). A one-line file
        took 80 ms.

        I've attached the test suite I used in case anyone else wants to play with
        this. Run "ant run-test" and "ant run-test2".

        Show
        Will Glass-Husain added a comment - Well, I ran the test suite again. To my embarassment, there appears to be almost no difference between Velocity with the decimal patch and without. It appears I took the version of Velocity previously compiled in the 1.4 release and compared it with one compiled on my machine. When I ran the latest source (compiled in the same manner) with and without the decimal patch, the results were identical. 10000 merges of an integer math heavy file took an average of 1532 ms (without the patch) and 1537 ms (with the patch). A one-line file took 80 ms. I've attached the test suite I used in case anyone else wants to play with this. Run "ant run-test" and "ant run-test2".
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=14146)
        complete number patch, replaces previous version

        This patch replaces previous versions. You'll also need to delete
        ASTNumberLiteral.java

        Show
        Will Glass-Husain added a comment - Created an attachment (id=14146) complete number patch, replaces previous version This patch replaces previous versions. You'll also need to delete ASTNumberLiteral.java
        Hide
        Will Glass-Husain added a comment -

        On second note, the performance test was too big to attach. Let me know if
        you'd like it and I'll email it to you.

        Show
        Will Glass-Husain added a comment - On second note, the performance test was too big to attach. Let me know if you'd like it and I'll email it to you.
        Hide
        Claude Brisson added a comment -

        That's a very good news. Anyway, your previous results were sufficiently strange
        to suspect something was wrong.

        Show
        Claude Brisson added a comment - That's a very good news. Anyway, your previous results were sufficiently strange to suspect something was wrong.
        Hide
        Will Glass-Husain added a comment -

        Yes, I wish all my programming headaches would quietly vanish after sitting
        untouched for a week.

        Show
        Will Glass-Husain added a comment - Yes, I wish all my programming headaches would quietly vanish after sitting untouched for a week.
        Hide
        Will Glass-Husain added a comment -

        Committed. Revision # 151484.

        Note that there's some small changes to the user's guide that need to be
        translated. See bug 33408.

        Show
        Will Glass-Husain added a comment - Committed. Revision # 151484. Note that there's some small changes to the user's guide that need to be translated. See bug 33408.
        Hide
        Henning Schmiedehausen added a comment -

        Close resolved issues for Release 1.5

        Show
        Henning Schmiedehausen added a comment - Close resolved issues for Release 1.5

          People

          • Assignee:
            Unassigned
            Reporter:
            Peter Romianowski
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development