Commons Math
  1. Commons Math
  2. MATH-1047

No check for overflow in "ArithmeticUtils.pow"

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.3
    • Labels:

      Description

      The "pow" methods in "o.a.c.m.util.ArithmeticUtils" do not check for overflow.
      They will happily return nonsensical results.

        Activity

        Gilles made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Gilles added a comment -

        Methods "pow(int,long)" and "pow(long,long)" are still unsafe but they are deprecated.

        Show
        Gilles added a comment - Methods "pow(int,long)" and "pow(long,long)" are still unsafe but they are deprecated .
        Hide
        Gilles added a comment - - edited

        Method "pow(long,int)" modified in revision 1537324 (using "mulAndCheck(long,long)").
        If a more efficient version is needed (possibly without safety check), it will need to be reimplemented with another name, and documentation that clearly indicates the purpose and caveat.

        Show
        Gilles added a comment - - edited Method "pow(long,int)" modified in revision 1537324 (using "mulAndCheck(long,long)"). If a more efficient version is needed (possibly without safety check), it will need to be reimplemented with another name, and documentation that clearly indicates the purpose and caveat.
        Hide
        Gilles added a comment -

        Method "pow(int,int)" modified in revision 1536683 (using "mulAndCheck").

        Show
        Gilles added a comment - Method "pow(int,int)" modified in revision 1536683 (using "mulAndCheck").
        Hide
        Gilles added a comment - - edited

        Maybe we should also take there the counterpart from guava

        No idea.

        In the meantime, I'd rather add the checks in the way that looks least disruptive.
        If performance was a significant target for this method, maybe there should be an associated test; then we can test alternate implementations. But none should be allowed to return nonsense.

        Show
        Gilles added a comment - - edited Maybe we should also take there the counterpart from guava No idea. In the meantime, I'd rather add the checks in the way that looks least disruptive. If performance was a significant target for this method, maybe there should be an associated test; then we can test alternate implementations. But none should be allowed to return nonsense.
        Hide
        Thomas Neidhart added a comment -

        The code for mulAndCheck(long, long) is indeed quite complex. Maybe we should also take there the counterpart from guava LongMath.checkedMultiply if it passes our unit tests.

        Show
        Thomas Neidhart added a comment - The code for mulAndCheck(long, long) is indeed quite complex. Maybe we should also take there the counterpart from guava LongMath.checkedMultiply if it passes our unit tests.
        Hide
        Gilles added a comment -

        For pow(int,int), that's smarter, indeed.
        A small caveat is that a failing "mulAndCheck" will create a less meaningful message.

        For the methods involving "long", mulAndCheck(long, long) seems to contain more branches that cannot be optimized away. And more operations.
        Since the code is far from obvious (due to bit-fiddling), it seems that the intention was to make it as efficient as possible. Calling "mulAndCheck" might defeat the purpose in some contexts (where the developer would then prefer to write the operation using an explicit loop).

        Show
        Gilles added a comment - For pow(int,int) , that's smarter, indeed. A small caveat is that a failing "mulAndCheck" will create a less meaningful message. For the methods involving "long", mulAndCheck(long, long) seems to contain more branches that cannot be optimized away. And more operations. Since the code is far from obvious (due to bit-fiddling), it seems that the intention was to make it as efficient as possible. Calling "mulAndCheck" might defeat the purpose in some contexts (where the developer would then prefer to write the operation using an explicit loop).
        Hide
        Thomas Neidhart added a comment -

        Why not call mulAndCheck(int, int) instead of doing the overflow detection yourself?

        Show
        Thomas Neidhart added a comment - Why not call mulAndCheck(int, int) instead of doing the overflow detection yourself?
        Gilles made changes -
        Field Original Value New Value
        Attachment MATH-1047.patch [ 12610320 ]
        Hide
        Gilles added a comment -

        I've attached a patch.
        Please review. If it's OK, I'll modify the other methods in a similar way.

        Show
        Gilles added a comment - I've attached a patch. Please review. If it's OK, I'll modify the other methods in a similar way.
        Gilles created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Gilles
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development