RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Daniel Fuchs]

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 <jbluettduncan at 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 <patrick at 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 <patrick at 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 <patrick at 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
>>>
>>>
>>>
>