Daniel, Claes,
List.of() and Collections.emptyList() are not the same. The behaviours are
different. Moreover, immutable static factory methods return instances which are
value-based. I believe it also means we are not tied with unconditional
instantiation, and in case of empty collections/maps could probably return the
same object every time.
We should ask Stuart why it has been done like that in the first place. Maybe
out of concern people might synchronize of those objects? I don't know. Let's
say for now it's an implementation-specific detail.
> On 15 Sep 2016, at 12:35, Claes Redestad <claes.redes...@oracle.com> wrote:
>
> +1
>
> I don't mind List.of() aesthetically, but there are places where
> startup/footprint is important where Collections.emptyList()
> is simply superior, e.g., constituting permanent data structures
> such as the module graph during early bootstrap.
>
> I know there are some drawbacks to the singleton approach of
> Collections.emptyList() (forces a potentially expensive memory
> load instead of just quickly allocating something that most likely
> will be thrown away just as quick), but having a very limited set
> of known constants for such things should be hard to notice on
> any workload since they'll be very likely to be in a CPU cache.
>
> Also saves having a few extra classes and decreases chances for
> profile pollution when calling methods with both
> Collections.emptyList() and List.of().
>
> TL;DR: I think List.of() should be an alias for Collections.emptyList()
> (same for Set.of() <-> Collections.emptySet(), Map.of() ..).
>
> Sorry. :-)
>
> /Claes
>
> On 2016-09-15 13:06, Daniel Fuchs wrote:
>> Hi,
>>
>> I'm not sure I like the replacement of Collections.emptyList()
>> by List.of();
>> I find emptyList() more expressive (+ it always returns
>> the same instance).
>>
>> best regards,
>>
>> -- daniel
>>
>> On 15/09/16 11:46, Pavel Rappo wrote:
>>> Hi,
>>>
>>> I have had a look at the change. It looks good.
>>>
>>> Retrofitting Collections.unmodifiableXXX/Arrays.asList with Convenience
>>> Factory
>>> Methods requires extra carefulness as the factory methods are stricter than
>>> any
>>> of those. Not only do they provide unconditional non-modifiability [0], they
>>> also do not tolerate `null` elements.
>>>
>>> It looks like you took all that into account.
>>> Tiny little comments and suggestions.
>>>
>>> 1. It might be the case we no longer need this [1]:
>>>
>>> finderList.forEach(Objects::requireNonNull);
>>>
>>> as the List.of does the same thing for free. Though it might be okay to
>>> leave it
>>> as an explicit check.
>>>
>>> Also, could you please fix a typo here (the same file):
>>>
>>> * will be propogated to the caller of the resulting module finder's
>>> ^
>>> 2. An interesting question (though it's a completely different issue) is
>>> whether
>>> or not the `cookieHeader` list in the map should also be unmodifiable [2]?
>>>
>>> 3. Could you please also fix the same (copy-pasted?) typo in FileTreeWalker?
>>>
>>> if {@code options} is {@ocde null} or the options
>>> ^^
>>> 4. Please remove this line from the ZoneRules constructor's javadoc:
>>>
>>> @return the zone rules, not null
>>>
>>> 5. Maybe we should revisit javadoc on those fields in [3]:
>>>
>>> This <code>List</code> is {@linkplain Collections#unmodifiableList(List)
>>> unmodifiable}.
>>>
>>> Since it's no longer the case.
>>>
>>> 6. I couldn't find any evidence that `resolverFields` might contain `null`.
>>> Am I missing a javadoc or a line of code? Maybe we should talk to java.time
>>> folks to see if it's the case?
>>>
>>> 7. Try to not make lines longer than they were before: 80 characters. Unless
>>> it's really needed.
>>>
>>> Thanks,
>>> -Pavel
>>>
>>> --------------------------------------------------------------------------------
>>>
>>> [0] Compare List.of().remove(new Object()) with
>>> Collections.emptyList().remove(new Object())
>>> [1]
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.00/src/java.base/share/classes/java/lang/module/ModuleFinder.java.sdiff.html
>>> [2]
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.00/src/java.base/share/classes/java/net/CookieManager.java.sdiff.html
>>> [3]
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.00/src/java.base/share/classes/java/util/ResourceBundle.java.sdiff.html
>>>
>>>> On 15 Sep 2016, at 09:52, Jonathan Bluett-Duncan <jbluettdun...@gmail.com>
>>>> wrote:
>>>>
>>>> Thanks Patrick!
>>>>
>>>> Stuart, would you be happy to sponsor this change?
>>>>
>>>> If anyone else has any thoughts regarding this change, then I'm all ears.
>>>>
>>>> Bug link: https://bugs.openjdk.java.net/browse/JDK-8134373
>>>> Rationale for changes:
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-September/043484.html
>>>>
>>>>
>>>> Kind regards,
>>>> Jonathan
>>>>
>>>>
>>>> On 14 September 2016 at 21:55, Patrick Reinhart <patr...@reini.net> wrote:
>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> Here are your changes in a webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.00
>>>>>
>>>>> -Patrick
>>>>>
>>>>> Am 13.09.2016 um 14:45 schrieb Patrick Reinhart <patr...@reini.net>:
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> On 2016-09-13 02:13, Jonathan Bluett-Duncan wrote:
>>>>>
>>>>> Hi Patrick,
>>>>> Thank you very much for the offer to hold my patch for me!
>>>>>
>>>>>
>>>>> No problem.
>>>>>
>>>>> Is it common practice to send patches to others using PGP?
>>>>>
>>>>>
>>>>> No, this is my personal setting.
>>>>>
>>>>> Kind regards,
>>>>> Jonathan
>>>>> On 12 September 2016 at 21:08, Patrick Reinhart <patr...@reini.net> wrote:
>>>>>
>>>>> Hi Jonathan,
>>>>> As I just also wanted to help some more clean-up in the JDKs final phase,
>>>>> I
>>>>> could offer you to hold that patch. Just send it to me and I will prepare
>>>>> the webrev for you….
>>>>> -Patrick
>>>>>
>>>>>
>>>>> -Patrick
>>>>>
>>>>>
>>>>>
>>>
>>
>
