corretto / corretto-17

Amazon Corretto 17 is a no-cost, multi-platform, production-ready distribution of OpenJDK 17
GNU General Public License v2.0
201 stars 47 forks source link

Why 'Stream.toList()' call 'new ArrayList()' twice? #179

Closed PgmJun closed 3 months ago

PgmJun commented 3 months ago

SSIA.

I can't understand this architecture. plz explain reason for me..🥲

Stream.toList()

default List<T> toList() {
        return (List<T>) Collections.unmodifiableList(new ArrayList<>(Arrays.asList(this.toArray())));
    }

Arrays.asList()

public static <T> List<T> asList(T... a) {
        return new ArrayList<>(a);
    }
caojoshua commented 3 months ago

Adding the sources for reference Stream::toList() Arrays::asList()

I'm not sure. Seems like the new ArrayList in Stream::toList is unnecessary.

The only argument I could think of is that Arrays::asList() returns a List, and is not guaranteed to return an ArrayList. Maybe Stream::toList specifically wants that list to be an ArrayList, but I cannot think of a good reason why.

This code is in the @implSpec, but I suspect this can be changed as well.

@PgmJun my answer is that is probably an oversight and should not be there. I can test removing the redundant new ArrayList(). But if you would like you can contribute the change yourself.

PgmJun commented 3 months ago

@caojoshua

Thank you! I'll test it myself and if there's no problem, I'll contribute!

mrserb commented 3 months ago

Note that this are two different classes with the same name.

alvdavi commented 3 months ago

To expand on Sergey's answer, there are two classes here, java.util.ArrayList<T> and java.util.Arrays.ArrayList<T>, which can lead to the confusion. The second one is a private static class.

There is a reason why the toList method is implemented that way. Our intention is to have an unmodifiable list, but wrapping an instance of java.util.Arrays.ArrayList<T> is not enough guarantee, because the constructor for it picks the array passed to it (which could then still be accessed). By adding the extra call to new ArrayList<>(...) in Stream.toList we ensure a defensive copy of the array is done by the java.util.ArrayList constructor.

For that reason, the PR can't be merged. But even if this change made sense, it would be a change that would be better implemented on the OpenJDK source code upstream and not directly into Corretto.

PgmJun commented 3 months ago

@alvdavi

Oh it was a different class. I got it. Thank you for your answer