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
