2006/8/28, Daniel Fridlender <[EMAIL PROTECTED]>:
Hi Mikhail,
On 8/25/06, Mikhail Loenko <[EMAIL PROTECTED]> wrote:
> Hi Daniel
>
> 2006/8/25, Daniel Fridlender <[EMAIL PROTECTED]>:
> > On 8/23/06, Mikhail Loenko <[EMAIL PROTECTED]> wrote:
> > > Hi Daniel,
> > >
> > > I've tried the patch. 4 math tests failed. Could you please take a look?
> > >
> > > Thanks,
> > > Mikhail
> > >
> >
> > Hi Mikhail,
> >
> > You are right. Two of these four tests are due to typos in the
> > exception messages:
> >
> >
1)org.apache.harmony.tests.java.math.BigDecimalConstructorsTest.testConstrStringExponentIntegerMin(
> > BigDecimalConstructorsTest.java:497)
> > expected<"Scale out of range."> but was <"Scale out of range">
> >
> >
2)org.apache.harmony.tests.java.math.BigDecimalScaleOperationsTest.testMovePointRightException(
> > BigDecimalScaleOperationsTest.java:335)
> > expected<"Underflow"> but was <"UnderFlow">
> >
> > We corrected them (and a similar problem with "Overflow") with a new
> > patch "typos.diff.zip´" in H1208.
> >
> > The other two failing tests require some discussion.
> >
> >
3)org.apache.harmony.tests.java.math.BigDecimalConstructorsTest.testConstrBI(BigDecimalConstructorsTest.java:75)
> >
> > I believe the test is wrong in this case since according to the API
> > specification the NullPointerException will actually be thrown before
> > the try block. The test incorrectly expects it to be thrown in the
> > block.
> >
> > The RI also fails this test.
>
> I've just rerun it on RI and it passed. What RI version do you use?
>
> I got the argument about the spec. It in a class description, right?
Right. The description of the class BigDecimal contains the statement
"All methods and constructors for this class throw
NullPointerException when passed a null object reference for any input
parameter."
Thus, there is a bug in the 1.5.0 version of the RI when invoking the
constructor BigDecimal(BigInteger val) with a null argument since it
does not throw NPE. This bug is fixed in version 1.5.0_08 (and also
in 1.6). I have just seen in H784 that Vladimir Ivanov discovered
this for version 1.5.0_06.
> >
4)org.apache.harmony.tests.java.math.BigDecimalArithmeticTest.testDivideByZero(BigDecimalArithmeticTest.java:492)
> > expected <"BigInteger divide by zero"> but was <"Division by zero">
> >
> > This is not just a typo. Both exception messages are used by the RI
> > in different BigDecimal.divide methods when dividing by
> > BigDecimal.ZERO. There is at least a third message: "/ by zero".
> > Some methods seem to use different messages depending on the size of
> > the dividend.
> >
> > So, I see two possibilities here:
> >
> > a) use everywhere the message "Division by zero".
> > b) try to find out when RI uses each of the different messages.
> >
> > I would suggest a) rather than b) since from the point of view of the
> > programmer it is reasonable to obtain the same message when dividing
> > by BigDecimal.ZERO regardless the size (or internal representation) of
> > the dividend and the method invoked for the division.
>
> In general if a programmer use only one method in their implementation where
> RI throws "BigInteger divide by zero", then they are not aware of
> other exception
> messages and they might hard code that "BigInteger divide by zero" one.
It is a bit more complicated than that. From the tests we have run we
can naturally guess that there are two groups of BigDecimal.divide
methods in RI:
i) those which check if the divisor is BigDecimal.ZERO in which case
they throw the exception with the message "Division by zero".
ii) those which do not check (forget to check?) and attempt to divide
anyhow: they will use the "BigInteger divide by zero" or "/ by zero"
depending on where the problem happens. That is, depending on whether
the dividend is large (and the division takes place in BigInteger) or
small (and the division takes place in a primitive type).
>
> So I'd prefer to keep it look like RI as it is now. BUT, if this change
implies
> an undesirable design change then I'm OK with discrepancy in exception
> messages
Keeping it look like RI for methods of group ii) would imply trying to
determine the exact border between messages "BigInteger divide by
zero" and "/ by zero".
OK I see. Let's keep it as is for now. Probably we'll later find some
logic behind
RI's exception messages. Anyway exception message compatibility is just a
nice-to-have feature and we have lots of other works to be done.
Thanks,
Mikhail
Thanks
Daniel
>
> Thanks,
> Mikhail
>
>
> >
> > In addition, doing b) may imply inspecting the behavior of RI a bit
> > too far. I am reluctant to do this since different messages may
> > reveal when the unscaled value is implemented as a BigInteger and when
> > as a primitive integral value.
> >
> > We submitted a new patch "tests.diff.zip" to H1208 correcting the test
> > mentioned in 3) and replacing the test in 4) by another invocation to
> > divide with the "Division by zero" message.
> >
> > Both RI and our implementation pass the tests.
> >
> > I think both patches should be applied. In the meantime we may
> > discuss if the approach b) is preferred.
> >
> > Regards,
> >
> > Daniel
> >
> > > 2006/8/22, Daniel Fridlender <[EMAIL PROTECTED]>:
> > > > Hi Mikhail,
> > > >
> > > > On 8/17/06, Mikhail Loenko <[EMAIL PROTECTED]> wrote:
> > > > > Hi Daniel,
> > > > >
> > > > > Please resubmit the patch with license granted to ASF
> > > >
> > > > done.
> > > >
> > > > >
> > > > > Seems like the patch was generated against original H-935 contribution
> > > > > and does not take into account later changes. Could you please
regenerate the
> > > > > patch?
> > > >
> > > > The new patch should be applied to the version of java.math currently
> > > > in the svn repository. It includes performance update from Vladimir
> > > > (with bugs fixed) and other similar optimizations. We tried to
> > > > respect spacing and line breaking from the version of the repository
> > > > (it often disagrees with the original H935).
> > > >
> > > > Thanks,
> > > >
> > > > Daniel
> > > >
> > > > >
> > > > > Thanks,
> > > > > Mikhail
> > > > >
> > > > > 2006/8/17, Mikhail Loenko (JIRA) <[EMAIL PROTECTED]>:
> > > > > > [ http://issues.apache.org/jira/browse/HARMONY-1208?page=all ]
> > > > > >
> > > > > > Mikhail Loenko reassigned HARMONY-1208:
> > > > > > ---------------------------------------
> > > > > >
> > > > > > Assignee: Mikhail Loenko
> > > > > >
> > > > > > > Bug fixing and cosmetics for Harmony 935
> > > > > > > ----------------------------------------
> > > > > > >
> > > > > > > Key: HARMONY-1208
> > > > > > > URL:
http://issues.apache.org/jira/browse/HARMONY-1208
> > > > > > > Project: Harmony
> > > > > > > Issue Type: Improvement
> > > > > > > Reporter: Daniel Fridlender
> > > > > > > Assigned To: Mikhail Loenko
> > > > > > > Priority: Minor
> > > > > > > Attachments: MathDiff.diff.zip
> > > > > > >
> > > > > > >
> > > > > > > A patch to fix some bugs in Harmony 935, turn serialization
compatible with RI and some cosmetics.
> > > > > >
> > > > > > --
> > > > > > This message is automatically generated by JIRA.
> > > > > > -
> > > > > > If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa
> > > > > > -
> > > > > > For more information on JIRA, see:
http://www.atlassian.com/software/jira
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > > >
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > For additional commands, e-mail: [EMAIL PROTECTED]
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > For additional commands, e-mail: [EMAIL PROTECTED]
> >
> >
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
