harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mikhail Loenko" <mloe...@gmail.com>
Subject Re: [jira] Assigned: (HARMONY-1208) Bug fixing and cosmetics for Harmony 935
Date Thu, 31 Aug 2006 09:48:14 GMT
2006/8/28, Daniel Fridlender <dfridlender@gmail.com>:
> Hi Mikhail,
>
> On 8/25/06, Mikhail Loenko <mloenko@gmail.com> wrote:
> > Hi Daniel
> >
> > 2006/8/25, Daniel Fridlender <dfridlender@gmail.com>:
> > > On 8/23/06, Mikhail Loenko <mloenko@gmail.com> 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 <dfridlender@gmail.com>:
> > > > > Hi Mikhail,
> > > > >
> > > > > On 8/17/06, Mikhail Loenko <mloenko@gmail.com> 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) <jira@apache.org>:
> > > > > > >     [ 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: harmony-dev-unsubscribe@incubator.apache.org
> > > > > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > > > > >
> > > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > > > >
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>

---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org


Mime
View raw message