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
>>
>>
>>
