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

Pavel Rappo pavel.rappo at oracle.com
Thu Sep 15 10:46:17 UTC 2016


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



More information about the core-libs-dev mailing list