[8u] RFR 8214687: Optimize Collections.nCopies().hashCode() and equals()

[Tagir Valeev]

To me, this change looks perfectly ok. I agree with Aleksey that adding a
dependency to Preconditions would be a worse solution. Yes, the proposed
solution violates the List::get contract, in particular, because
AssertionError is thrown instead of IndexOutOfBoundsException. However,
it's a test-case code and this check normally never fails. It can fail due
to a bug, and I find it quite ok if the test-case fails with an
AssertionError as a reaction to the bug. In any case, I don't think that we
should spend too much time polishing this particular line.

With best regards,
Tagir Valeev.

On Wed, Aug 7, 2019 at 10:17 PM Aleksey Shipilev <shade at redhat.com> wrote:

> On 7/31/19 6:08 PM, Aleksey Shipilev wrote:
> > On 7/31/19 5:56 PM, Andrew John Hughes wrote:
> >> On 31/07/2019 10:02, Aleksey Shipilev wrote:
> >>> On 7/16/19 9:25 AM, Andrew John Hughes wrote:
> >>>> On 15/07/2019 09:27, Aleksey Shipilev wrote:
> >>> Getting back to this. Since we have moved Preconditions to private
> location in 8u, using it is
> >>> impossible for this patch. So, I would keep the webrev as is:
> >>>   https://cr.openjdk.java.net/~shade/8214687/webrev.8u.01
> >>
> >> We haven't moved anything as yet and my proposed patch leaves the class
> >> as public, so shouldn't be a blocker. The test in that patch uses
> >> Preconditions.
> >
> > Original patch needs Objects.checkIndex:
> >   https://hg.openjdk.java.net/jdk/jdk/rev/cfceb4df2499#l2.32
> >
> > What you are suggesting is changing that line to Preconditions (with new
> method!). What webrev
> > suggests is replacing it with the one-liner:
> >
> >   91  check(0 <= index && index < n, "Index is incorrect");
> >
> > Given that we are changing the code anyway, I don't see why do we need
> to introduce another
> > dependency to the about-to-be-moved class, do more code that diverges 8u
> from later releases,
> > instead of doing the trivial one-liner.
>
> Thoughts?
>
> --
> Thanks,
> -Aleksey
>
>