Le mer. 8 avr. 2020 à 15:58, Alex Herbert <alex.d.herb...@gmail.com> a écrit :
>
>
> On 08/04/2020 14:08, Gilles Sadowski wrote:
> > Le mer. 8 avr. 2020 à 14:26, Alex Herbert <alex.d.herb...@gmail.com> a
> > écrit :
> >>
> >> On 08/04/2020 00:36, Gilles Sadowski wrote:
> >>> 2020-04-07 23:01 UTC+02:00, Alex Herbert <alex.d.herb...@gmail.com>:
> >>>>> On 7 Apr 2020, at 17:47, Gilles Sadowski <gillese...@gmail.com> wrote:
> >>>>>
> >>>>> Le mar. 7 avr. 2020 à 14:54, Alex Herbert <alex.d.herb...@gmail.com
> >>>>> <mailto:alex.d.herb...@gmail.com>> a écrit :
> >>>>>> On 07/04/2020 13:43, Alex Herbert wrote:
> >>>>>>> Fraction
> >>>>>>>
> >>>>>>> I also did a big arrangement of Fraction and BigFraction.
> >>>>> Thanks.
> >>>>>
> >>>>>> I noticed that hashCode is using some variant of the format used in
> >>>>>> Arrays.hashCode(...)
> >>>>>>
> >>>>>> I wondered if we should standardise on returning a value as if computed
> >>>>>> using e.g. Arrays.hashCode(new int[]{numerator, denominator}) as was
> >>>>>> done for Complex with its two parts.
> >>>>>>
> >>>>>> This would change in Fraction:
> >>>>>>
> >>>>>> public int hashCode() {
> >>>>>> return 37 * (37 * 17 + numerator) + denominator;
> >>>>>> }
> >>>>> Strange that the constant 37 * 17 was not pre-computed. ;-)
> >>>> Yes. Weird. It was like that in CM 3.6.1.
> >>>>
> >>>> Not knowing if one is better than the other
> >>> The "17" seems to come from "Effective Java" (where J. Bloch says
> >>> that it is arbitrary apart from being nonzero, so "1" is fine too I
> >>> guess).
> >>>
> >>>> (do you just pick a small prime
> >>>> for the factor?) I’d just shift it to use the same method as
> >>>> Arrays.hashCode
> >>>> for consistency with Complex.
> >>> Fine.
> >>> The factor is indeed "31" in Effective Java, saying that it must be
> >>> odd, and that a prime is a traditional choice.
> >> hashCode was actually broken so it needed an update.
> >>
> >> These
> >>
> >> -1 / 3
> >>
> >> 1 / -3
> >>
> >> did not have the same hashCode even though they are equal. This violates
> >> the Object.hashCode contract.
> > Oops.
> > I overlooked that when we decided to allow both internal representations.
> >
> >> There were two options:
> >>
> >> 1. Create the hashCode using: |numerator|, |denominator|
> >>
> >> 2. Create the hashCode using: signum, |numerator|, |denominator|
> >>
> >> I went for option 2. Thus 1/3 and -1/3 have different hash codes.
> >>
> >> The hash code computation currently matches (for Fraction f):
> >>
> >> Arrays.hashCode(new int[] {f.signum(),
> >> Math.abs(f.getNumerator()),
> >> Math.abs(f.getDenominator())});
> >>
> >> The computation is done inline without delegating to Arrays.
> > I'd rather keep it simple and delegate to the JDK method above.
>
> I've documented the method to state what it is doing.
>
> Avoiding Arrays.hashCode will be faster due to lack of array
> instantiation and then boundary checking on the array during
> computation. In the case of BigFraction the method also avoids having to
> compute the abs() of the numerator and denominator.
>
> If you ever did need to have a hashCode then it is nicer to have a
> faster method.
Of course, but I can't come up with a use-case for "Set<Fraction>"
which you mentioned below.
If the rationale is "We do <...> every time we need to define a hashCode".
Then +1 to keep doing <...>.
> It you argue for Arrays.hashCode(Object[]) then you could also argue for
> Arrays.equals(Object[], Object[]) in the equals method using the sign,
> and absolute numerator and denominator. I think it better leave it
> functionally the same but implement the method explicitly to take
> advantage of knowing the internal object representation.
I'd assume that it's always the case. Then IIUC, the JDK methods
only exists for computing reference values in unit tests...
> I stopped short of adding this implementation detail to the javadoc for
> Fraction. However in Complex we mandated in the javadoc that equals and
> hashCode are equivalent to using Arrays.equals/hashCode with the two
> parts of the complex number.
>
> > Indeed, is there any reasonable use-case for using a fraction
> > as hash map key?
> > If not, IMO, better make the code self-documenting.
>
> Storage in a Set<Fraction> for unique values?
Not use-case, just an example.
Anyways, not strong arguments here; and absence of actual
usage would make it a total waste of time to set up benchmarks.
Do what you think is fine.
Best,
Gilles
> >> It could be changed to:
> >>
> >> f.signum() * Arrays.hashCode(new int[] {Math.abs(f.getNumerator()),
> >> Math.abs(f.getDenominator())});
> > -0
> > Same rationale as above.
> >
> >> The later would remove a single integer addition operation from the
> >> computation so probably no speed difference.
> > Zero difference if "hashCode()" is never called. ;-)
> >
> > Gilles
> >
> >> Any opinions on this? Given the signum is a 3-state flag it may be
> >> better moved outside the computation. This would mean that a fraction
> >> with the value 0 has the hashCode 0.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
