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. :-)


On 2016-09-15 13:06, Daniel Fuchs wrote:

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:

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


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.


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

Kind regards,

On 14 September 2016 at 21:55, Patrick Reinhart <patr...@reini.net> wrote:

Hi Jonathan,

Here are your changes in a webrev:



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,
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….


Reply via email to