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

[Jonathan Bluett-Duncan]

Yes, you're very welcome Stuart. :-)

Kind regards,
Jonathan

On 12 Oct 2016 21:49, "Patrick Reinhart"  wrote:

You’re welcome :-)

-Patrick

Am 12.10.2016 um 22:41 schrieb Stuart Marks :

Tests passed, spec change approved, changeset pushed:

http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af71f6a36731

Jonathan, thanks for your contribution. And Patrick, thanks again for
hosting the webrev.

s'marks

On 10/12/16 6:53 AM, Jonathan Bluett-Duncan wrote:

Hi all,

Thank you very much for all reviewing my changeset over the last few days
and for finding the bits I forgot to transfer from webrev01 to webrev02.
I've been quiet as I'm still busy with university and will be for the
foreseeable future.

Stuart, many thanks again for sponsoring the change and going through the
whole procedure for me. I look forward to the rest of your results.

Kind regards,
Jonathan

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

[Patrick Reinhart]

You’re welcome :-)

-Patrick

> Am 12.10.2016 um 22:41 schrieb Stuart Marks :
> 
> Tests passed, spec change approved, changeset pushed:
> 
> http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af71f6a36731 
> 
> Jonathan, thanks for your contribution. And Patrick, thanks again for hosting 
> the webrev.
> 
> s'marks
> 
> On 10/12/16 6:53 AM, Jonathan Bluett-Duncan wrote:
>> Hi all,
>> 
>> Thank you very much for all reviewing my changeset over the last few days 
>> and for finding the bits I forgot to transfer from webrev01 to webrev02. 
>> I've been quiet as I'm still busy with university and will be for the 
>> foreseeable future.
>> 
>> Stuart, many thanks again for sponsoring the change and going through the 
>> whole procedure for me. I look forward to the rest of your results.
>> 
>> Kind regards,
>> Jonathan
>> 
> 

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

[Stuart Marks]

Tests passed, spec change approved, changeset pushed:

http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af71f6a36731

Jonathan, thanks for your contribution. And Patrick, thanks again for hosting 
the webrev.


s'marks


On 10/12/16 6:53 AM, Jonathan Bluett-Duncan wrote:

Hi all,

Thank you very much for all reviewing my changeset over the last few days and 
for finding the bits I forgot to transfer from webrev01 to webrev02. I've been 
quiet as I'm still busy with university and will be for the foreseeable future.


Stuart, many thanks again for sponsoring the change and going through the 
whole procedure for me. I look forward to the rest of your results.


Kind regards,
Jonathan

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

[Stuart Marks]

Hi Naoto,

Great, thanks for confirming this.

s'marks

On 10/11/16 4:13 PM, Naoto Sato wrote:

+1 for removing the link to Collections#unmodifiableList in the spec. I don't
see any particular reason to specify in the javadoc and don't believe it would
break any existing apps.

Naoto

On 10/11/16 4:03 PM, Stuart Marks wrote:



On 10/10/16 11:26 PM, Andrej Golovnin wrote:

src/java.base/share/classes/java/util/ResourceBundle.java

2490 public static class Control {
2491 /**
2492  * The default format List, which contains
the strings
2493  * "java.class" and
"java.properties", in
2494  * this order. This List is {@linkplain
2495  * Collections#unmodifiableList(List) unmodifiable}.
2496  *
2497  * @see #getFormats(String)
2498  */
2499 public static final List FORMAT_DEFAULT =
List.of("java.class", "java.properties");

I think you should also change the JavaDocs in the
ResourceBundle.Control class for the constants FORMAT_CLASS,
FORMAT_DEFAULT and FORMAT_PROPERTIES, because the JavaDocs for this
constants explicitly mentions, that the lists are created by using
Collections#unmodifiableList(List). Or you cannot change this
constants at all because they are part of the Public API and existed
in this form for a long time. Please ask someone from Oracle for help.
They can explain it better when it is OK to change and when not. Maybe
Stuart can do that.


Hi Andrej,

Thanks for pointing this out.

It appears that the changes to remove the links to
Collections.unmodifiableList() was dropped from webrev.01 to webrev.02.
I've restored them, along with a couple other changes that were also
dropped. I also updated the ModuleFinder.java comment per a request from
Alan Bateman. The revised webrev is here:

http://cr.openjdk.java.net/~smarks/reviews/8134373/webrev.03/

In any case, yes, the specifications of the ResourceBundle.Control
fields should be changed to remove the links to
Collections.unmodifiableList(). It's unclear whether having a link this
way implies that it's part of the specification that these fields must
be instances returned from that method. Removing the link makes it clear
that saying the List is unmodifiable is merely a description of the
required behavior.

I spoke with Joe Darcy (our compatibility guru) and we agreed that out
of an abundance of caution it would be wise to file a request to make
this change. (This is the "CCC" - basically an internal change control
process for Java SE specifications.) Doing this is mainly for tracking
purposes, as we believe this to be a compatible change.

I've also included in this request a mention of the change to
CookieManager.get() which we had discussed previously. Even though we
believe this is also a compatible change, it's also a change that should
be tracked.

I'll follow up when this bit of process is finished.

s'marks

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

[Jonathan Bluett-Duncan]

Hi all,

Thank you very much for all reviewing my changeset over the last few days
and for finding the bits I forgot to transfer from webrev01 to webrev02.
I've been quiet as I'm still busy with university and will be for the
foreseeable future.

Stuart, many thanks again for sponsoring the change and going through the
whole procedure for me. I look forward to the rest of your results.

Kind regards,
Jonathan

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

[Andrej Golovnin]

Hi Stuart,

> It appears that the changes to remove the links to
> Collections.unmodifiableList() was dropped from webrev.01 to webrev.02. I've
> restored them, along with a couple other changes that were also dropped. I
> also updated the ModuleFinder.java comment per a request from Alan Bateman.
> The revised webrev is here:
>
> http://cr.openjdk.java.net/~smarks/reviews/8134373/webrev.03/

looks good. Thanks!

Best regards,
Andrej Golovnin

>
> In any case, yes, the specifications of the ResourceBundle.Control fields
> should be changed to remove the links to Collections.unmodifiableList().
> It's unclear whether having a link this way implies that it's part of the
> specification that these fields must be instances returned from that method.
> Removing the link makes it clear that saying the List is unmodifiable is
> merely a description of the required behavior.
>
> I spoke with Joe Darcy (our compatibility guru) and we agreed that out of an
> abundance of caution it would be wise to file a request to make this change.
> (This is the "CCC" - basically an internal change control process for Java
> SE specifications.) Doing this is mainly for tracking purposes, as we
> believe this to be a compatible change.
>
> I've also included in this request a mention of the change to
> CookieManager.get() which we had discussed previously. Even though we
> believe this is also a compatible change, it's also a change that should be
> tracked.
>
> I'll follow up when this bit of process is finished.
>
> s'marks

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

[Naoto Sato]

+1 for removing the link to Collections#unmodifiableList in the spec. I 
don't see any particular reason to specify in the javadoc and don't 
believe it would break any existing apps.


Naoto

On 10/11/16 4:03 PM, Stuart Marks wrote:



On 10/10/16 11:26 PM, Andrej Golovnin wrote:

src/java.base/share/classes/java/util/ResourceBundle.java

2490 public static class Control {
2491 /**
2492  * The default format List, which contains
the strings
2493  * "java.class" and
"java.properties", in
2494  * this order. This List is {@linkplain
2495  * Collections#unmodifiableList(List) unmodifiable}.
2496  *
2497  * @see #getFormats(String)
2498  */
2499 public static final List FORMAT_DEFAULT =
List.of("java.class", "java.properties");

I think you should also change the JavaDocs in the
ResourceBundle.Control class for the constants FORMAT_CLASS,
FORMAT_DEFAULT and FORMAT_PROPERTIES, because the JavaDocs for this
constants explicitly mentions, that the lists are created by using
Collections#unmodifiableList(List). Or you cannot change this
constants at all because they are part of the Public API and existed
in this form for a long time. Please ask someone from Oracle for help.
They can explain it better when it is OK to change and when not. Maybe
Stuart can do that.


Hi Andrej,

Thanks for pointing this out.

It appears that the changes to remove the links to
Collections.unmodifiableList() was dropped from webrev.01 to webrev.02.
I've restored them, along with a couple other changes that were also
dropped. I also updated the ModuleFinder.java comment per a request from
Alan Bateman. The revised webrev is here:

http://cr.openjdk.java.net/~smarks/reviews/8134373/webrev.03/

In any case, yes, the specifications of the ResourceBundle.Control
fields should be changed to remove the links to
Collections.unmodifiableList(). It's unclear whether having a link this
way implies that it's part of the specification that these fields must
be instances returned from that method. Removing the link makes it clear
that saying the List is unmodifiable is merely a description of the
required behavior.

I spoke with Joe Darcy (our compatibility guru) and we agreed that out
of an abundance of caution it would be wise to file a request to make
this change. (This is the "CCC" - basically an internal change control
process for Java SE specifications.) Doing this is mainly for tracking
purposes, as we believe this to be a compatible change.

I've also included in this request a mention of the change to
CookieManager.get() which we had discussed previously. Even though we
believe this is also a compatible change, it's also a change that should
be tracked.

I'll follow up when this bit of process is finished.

s'marks

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

[Stuart Marks]

On 10/10/16 11:26 PM, Andrej Golovnin wrote:

src/java.base/share/classes/java/util/ResourceBundle.java

2490 public static class Control {
2491 /**
2492  * The default format List, which contains the strings
2493  * "java.class" and "java.properties", in
2494  * this order. This List is {@linkplain
2495  * Collections#unmodifiableList(List) unmodifiable}.
2496  *
2497  * @see #getFormats(String)
2498  */
2499 public static final List FORMAT_DEFAULT =
List.of("java.class", "java.properties");

I think you should also change the JavaDocs in the
ResourceBundle.Control class for the constants FORMAT_CLASS,
FORMAT_DEFAULT and FORMAT_PROPERTIES, because the JavaDocs for this
constants explicitly mentions, that the lists are created by using
Collections#unmodifiableList(List). Or you cannot change this
constants at all because they are part of the Public API and existed
in this form for a long time. Please ask someone from Oracle for help.
They can explain it better when it is OK to change and when not. Maybe
Stuart can do that.


Hi Andrej,

Thanks for pointing this out.

It appears that the changes to remove the links to 
Collections.unmodifiableList() was dropped from webrev.01 to webrev.02. I've 
restored them, along with a couple other changes that were also dropped. I also 
updated the ModuleFinder.java comment per a request from Alan Bateman. The 
revised webrev is here:


http://cr.openjdk.java.net/~smarks/reviews/8134373/webrev.03/

In any case, yes, the specifications of the ResourceBundle.Control fields should 
be changed to remove the links to Collections.unmodifiableList(). It's unclear 
whether having a link this way implies that it's part of the specification that 
these fields must be instances returned from that method. Removing the link 
makes it clear that saying the List is unmodifiable is merely a description of 
the required behavior.


I spoke with Joe Darcy (our compatibility guru) and we agreed that out of an 
abundance of caution it would be wise to file a request to make this change. 
(This is the "CCC" - basically an internal change control process for Java SE 
specifications.) Doing this is mainly for tracking purposes, as we believe this 
to be a compatible change.


I've also included in this request a mention of the change to 
CookieManager.get() which we had discussed previously. Even though we believe 
this is also a compatible change, it's also a change that should be tracked.


I'll follow up when this bit of process is finished.

s'marks

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

[Andrej Golovnin]

Hi Jonathan,

src/java.base/share/classes/java/util/ResourceBundle.java

2490 public static class Control {
2491 /**
2492  * The default format List, which contains the strings
2493  * "java.class" and "java.properties", in
2494  * this order. This List is {@linkplain
2495  * Collections#unmodifiableList(List) unmodifiable}.
2496  *
2497  * @see #getFormats(String)
2498  */
2499 public static final List FORMAT_DEFAULT =
List.of("java.class", "java.properties");

I think you should also change the JavaDocs in the
ResourceBundle.Control class for the constants FORMAT_CLASS,
FORMAT_DEFAULT and FORMAT_PROPERTIES, because the JavaDocs for this
constants explicitly mentions, that the lists are created by using
Collections#unmodifiableList(List). Or you cannot change this
constants at all because they are part of the Public API and existed
in this form for a long time. Please ask someone from Oracle for help.
They can explain it better when it is OK to change and when not. Maybe
Stuart can do that.

Best regards,
Andrej Golovnin

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

[Stuart Marks]

OK, I'll sponsor this. I need to run this through our internal testing system 
before pushing it. I'll follow up here with results.


s'marks


On 10/10/16 1:34 AM, Jonathan Bluett-Duncan wrote:

Hi all,

Would you kindly review the latest webrev now?

http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.02 



Thanks in advance.

Kind regards,
Jonathan

On 7 October 2016 at 21:59, Patrick Reinhart > wrote:


Here is the latest webrev:

http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.02


-Patrick



> Am 07.10.2016 um 12:00 schrieb Jonathan Bluett-Duncan
>:
>
> Hi all,
>
> Sorry for the delayed response, I've been busy lately with university
and other things.
>
> Thank you all for your comments. I'll leave the DateTimeFormatter
comment in, as you requested Stephen and Roger, and I'll work again with
Patrick as soon as I'm ready.
>
> Kind regards,
> Jonathan
>
> On 6 October 2016 at 09:38, Stephen Colebourne > wrote:
> On 6 October 2016 at 00:00, Stuart Marks > wrote:
> >> I think you should perform no change to DateTimeFormatter (other than
> >> a comment) as part of this changeset. The behaviour of that
> >> DateTimeFormatter method is subtle, and I now suspect that what we
> >> have there might be the best option.
> >
> > I had recommended removing the comment from DateTimeFormatter, but if
> > Stephen wants the comment in, that's fine with me. In fact I'll defer to
> > what Stephen (and Roger Riggs) want with this code, since they're the
> > maintainers.
>
> I think it makes sense to leave the new comment in. All further change
> should be under 8167222.
>
> Stephen
>

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

[Jonathan Bluett-Duncan]

Hi all,

Would you kindly review the latest webrev now?

http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.02

Thanks in advance.

Kind regards,
Jonathan

On 7 October 2016 at 21:59, Patrick Reinhart  wrote:

> Here is the latest webrev:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.02
>
> -Patrick
>
>
>
> > Am 07.10.2016 um 12:00 schrieb Jonathan Bluett-Duncan <
> jbluettdun...@gmail.com>:
> >
> > Hi all,
> >
> > Sorry for the delayed response, I've been busy lately with university
> and other things.
> >
> > Thank you all for your comments. I'll leave the DateTimeFormatter
> comment in, as you requested Stephen and Roger, and I'll work again with
> Patrick as soon as I'm ready.
> >
> > Kind regards,
> > Jonathan
> >
> > On 6 October 2016 at 09:38, Stephen Colebourne 
> wrote:
> > On 6 October 2016 at 00:00, Stuart Marks 
> wrote:
> > >> I think you should perform no change to DateTimeFormatter (other than
> > >> a comment) as part of this changeset. The behaviour of that
> > >> DateTimeFormatter method is subtle, and I now suspect that what we
> > >> have there might be the best option.
> > >
> > > I had recommended removing the comment from DateTimeFormatter, but if
> > > Stephen wants the comment in, that's fine with me. In fact I'll defer
> to
> > > what Stephen (and Roger Riggs) want with this code, since they're the
> > > maintainers.
> >
> > I think it makes sense to leave the new comment in. All further change
> > should be under 8167222.
> >
> > Stephen
> >
>
>

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

[Patrick Reinhart]

Here is the latest webrev:

http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.02

-Patrick



> Am 07.10.2016 um 12:00 schrieb Jonathan Bluett-Duncan 
> :
> 
> Hi all,
> 
> Sorry for the delayed response, I've been busy lately with university and 
> other things.
> 
> Thank you all for your comments. I'll leave the DateTimeFormatter comment in, 
> as you requested Stephen and Roger, and I'll work again with Patrick as soon 
> as I'm ready.
> 
> Kind regards,
> Jonathan
> 
> On 6 October 2016 at 09:38, Stephen Colebourne  wrote:
> On 6 October 2016 at 00:00, Stuart Marks  wrote:
> >> I think you should perform no change to DateTimeFormatter (other than
> >> a comment) as part of this changeset. The behaviour of that
> >> DateTimeFormatter method is subtle, and I now suspect that what we
> >> have there might be the best option.
> >
> > I had recommended removing the comment from DateTimeFormatter, but if
> > Stephen wants the comment in, that's fine with me. In fact I'll defer to
> > what Stephen (and Roger Riggs) want with this code, since they're the
> > maintainers.
> 
> I think it makes sense to leave the new comment in. All further change
> should be under 8167222.
> 
> Stephen
> 

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

[Jonathan Bluett-Duncan]

Hi all,

Sorry for the delayed response, I've been busy lately with university and
other things.

Thank you all for your comments. I'll leave the DateTimeFormatter comment
in, as you requested Stephen and Roger, and I'll work again with Patrick as
soon as I'm ready.

Kind regards,
Jonathan

On 6 October 2016 at 09:38, Stephen Colebourne  wrote:

> On 6 October 2016 at 00:00, Stuart Marks  wrote:
> >> I think you should perform no change to DateTimeFormatter (other than
> >> a comment) as part of this changeset. The behaviour of that
> >> DateTimeFormatter method is subtle, and I now suspect that what we
> >> have there might be the best option.
> >
> > I had recommended removing the comment from DateTimeFormatter, but if
> > Stephen wants the comment in, that's fine with me. In fact I'll defer to
> > what Stephen (and Roger Riggs) want with this code, since they're the
> > maintainers.
>
> I think it makes sense to leave the new comment in. All further change
> should be under 8167222.
>
> Stephen
>

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

[Stephen Colebourne]

On 6 October 2016 at 00:00, Stuart Marks  wrote:
>> I think you should perform no change to DateTimeFormatter (other than
>> a comment) as part of this changeset. The behaviour of that
>> DateTimeFormatter method is subtle, and I now suspect that what we
>> have there might be the best option.
>
> I had recommended removing the comment from DateTimeFormatter, but if
> Stephen wants the comment in, that's fine with me. In fact I'll defer to
> what Stephen (and Roger Riggs) want with this code, since they're the
> maintainers.

I think it makes sense to leave the new comment in. All further change
should be under 8167222.

Stephen

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

[Stuart Marks]

On 10/5/16 12:32 PM, Stephen Colebourne wrote:

On 5 October 2016 at 17:07, Jonathan Bluett-Duncan
 wrote:

Stephen, thank you for getting back about DateTimeFormatter. It's not clear
to me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
delete it; or do I change it to test that a null TemporalField param causes
a NullPointerException to be thrown; or do I do something else? May I have
your continued thoughts on this?


I think you should perform no change to DateTimeFormatter (other than
a comment) as part of this changeset. The behaviour of that
DateTimeFormatter method is subtle, and I now suspect that what we
have there might be the best option.


I had recommended removing the comment from DateTimeFormatter, but if Stephen 
wants the comment in, that's fine with me. In fact I'll defer to what Stephen 
(and Roger Riggs) want with this code, since they're the maintainers.


s'marks

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

[Roger Riggs]

Hi,

I would leave the test and code as is for the purposes of 8133273
and leave it to later to resolve 8167222.  There is no compelling need 
to change it.


$.02, Roger

On 10/5/2016 4:45 PM, Stuart Marks wrote:

Stephen,

Thanks for the quick followup clarifications. I'm not entirely sure 
how you'd like to proceed; see discussion below.


Jonathan, also see below.


On 10/5/16 9:07 AM, Jonathan Bluett-Duncan wrote:
Stuart, thank you very much for your continued review of this 
changeset, and for
finding those usages of CookieManager::get in Grepcode for me. I've 
applied the

comment you suggested for ModuleFinder and I've also fixed the
NetscapeCookieStore typo.


Great!

Stephen, thank you for getting back about DateTimeFormatter. It's not 
clear to

me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. 
Do I
delete it; or do I change it to test that a null TemporalField param 
causes a
NullPointerException to be thrown; or do I do something else? May I 
have your

continued thoughts on this?


OK, this is kind of subtle. This is a TCK (conformance) test, so it 
probably cannot simply be removed; it may need a specification change 
to clarify this case. I've filed JDK-8167222 to cover these issues. 
I've made a note in this bug regarding the potential change in 
DateTimeFormatter.withResolverFields() to use Set.of().


(There's an additional wrinkle with Set.of() aside from rejecting 
nulls; it also rejects duplicates by throwing IAE.)


In any case that code can't be changed to use Set.of() until the 
test/spec issue is resolved, so for the purposes of this changeset, 
I'd suggest simply removing the withResolverFields() comment from the 
webrev. We can revisit this during or after the resolution of 
JDK-8167222.


I think this clears all the issues, so you can probably go ahead and 
work with Patrick to update the webrev. And Patrick, thanks once again 
for hosting the webrev!


s'marks

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

[Stuart Marks]

Stephen,

Thanks for the quick followup clarifications. I'm not entirely sure how you'd 
like to proceed; see discussion below.


Jonathan, also see below.


On 10/5/16 9:07 AM, Jonathan Bluett-Duncan wrote:

Stuart, thank you very much for your continued review of this changeset, and for
finding those usages of CookieManager::get in Grepcode for me. I've applied the
comment you suggested for ModuleFinder and I've also fixed the
NetscapeCookieStore typo.


Great!


Stephen, thank you for getting back about DateTimeFormatter. It's not clear to
me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
delete it; or do I change it to test that a null TemporalField param causes a
NullPointerException to be thrown; or do I do something else? May I have your
continued thoughts on this?


OK, this is kind of subtle. This is a TCK (conformance) test, so it probably 
cannot simply be removed; it may need a specification change to clarify this 
case. I've filed JDK-8167222 to cover these issues. I've made a note in this bug 
regarding the potential change in DateTimeFormatter.withResolverFields() to use 
Set.of().


(There's an additional wrinkle with Set.of() aside from rejecting nulls; it also 
rejects duplicates by throwing IAE.)


In any case that code can't be changed to use Set.of() until the test/spec issue 
is resolved, so for the purposes of this changeset, I'd suggest simply removing 
the withResolverFields() comment from the webrev. We can revisit this during or 
after the resolution of JDK-8167222.


I think this clears all the issues, so you can probably go ahead and work with 
Patrick to update the webrev. And Patrick, thanks once again for hosting the webrev!


s'marks

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

[Stephen Colebourne]

On 5 October 2016 at 17:07, Jonathan Bluett-Duncan
 wrote:
> Stephen, thank you for getting back about DateTimeFormatter. It's not clear
> to me what should be done with
> TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
> delete it; or do I change it to test that a null TemporalField param causes
> a NullPointerException to be thrown; or do I do something else? May I have
> your continued thoughts on this?

I think you should perform no change to DateTimeFormatter (other than
a comment) as part of this changeset. The behaviour of that
DateTimeFormatter method is subtle, and I now suspect that what we
have there might be the best option.

Stephen

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

[Patrick Reinhart]

Hi Jonathan,

As soon you got all the changes together, we can go thru them and I will 
recreate the webrev…

-Patrick

> Am 05.10.2016 um 18:07 schrieb Jonathan Bluett-Duncan 
> :
> 
> Stuart, thank you very much for your continued review of this changeset,
> and for finding those usages of CookieManager::get in Grepcode for me. I've
> applied the comment you suggested for ModuleFinder and I've also fixed the
> NetscapeCookieStore typo.
> 
> Stephen, thank you for getting back about DateTimeFormatter. It's not clear
> to me what should be done with
> TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
> delete it; or do I change it to test that a null TemporalField param causes
> a NullPointerException to be thrown; or do I do something else? May I have
> your continued thoughts on this?
> 
> Kind regards,
> Jonathan

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

[Jonathan Bluett-Duncan]

Stuart, thank you very much for your continued review of this changeset,
and for finding those usages of CookieManager::get in Grepcode for me. I've
applied the comment you suggested for ModuleFinder and I've also fixed the
NetscapeCookieStore typo.

Stephen, thank you for getting back about DateTimeFormatter. It's not clear
to me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
delete it; or do I change it to test that a null TemporalField param causes
a NullPointerException to be thrown; or do I do something else? May I have
your continued thoughts on this?

Kind regards,
Jonathan

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

[Stephen Colebourne]

On 4 October 2016 at 23:27, Stuart Marks  wrote:
> 4) The 'resolverFields' problem/comment was regarding DateTimeFormatter (see
> this part of latest webrev), where I realised I couldn't use Set.of instead
> of Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I
> noticed that one of the java.time tests was testing whether
> DateTimeFormatter.withResolverFields(TemporalField...) could accept null
> parameters, which made using Set.of impossible since it's null-hostile (as
> in it throws NullPointerException upon being constructed with null
> element(s)).
>
> Hm, yes, it's odd that there's a test for an array containing a null, in
> addition to an empty array and a null array. See:
>
> jdk/test/java/time/tck/java/time/format/TCKDateTimeFormatter.java
> test_resolverFields_listOfOneNull()
>
> I'm not entirely sure what's intended here. In any case, let's wait until we
> hear from Mr. Colebourne.

I don't think that is a sensible test. AFAICT, null in the set has no
meaning. Its not documented to have meaning, so it is really a bug
with a test.

> 5) For the changes in the Chronology classes, the era() method now returns
> an immutable array where it didn't before. (The List returned by
> Arrays.asList() can have individual elements modified but its size can't be
> changed.) The spec for eras() says "may be immutable" so presumably this is
> OK. But note, since the returned List is now immutable, a new instance
> needn't be created each time. I'm not sure whether it's worth keeping around
> a cached copy in case someone calls eras() again though.
>
> It would be good if we could hear from Stephen on this one as well.

eras() will rarely be used, so no point caching. Changing to List.of() is fine.

Stephen

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

[Stuart Marks]

On 9/30/16 6:57 AM, Jonathan Bluett-Duncan wrote:
1) It actually didn't occur to me that there was a potential TOCTOU problem in 
ModuleFinder.compose, despite the fact that I found a potential problem in 
FileTreeIterator - it completely missed me, so thank you for finding it! I'll 
see if I can put a similar comment there to what I wrote in FileTreeIterator.

OK. A simple comment such as

// make a copy since it's captured by the ModuleFinder instance belowfinal 
List finderList = List.of(finders);

should be sufficient.
2) ... I decided just now to do a search on Grepcode for usages of 
CookieManager.get 
 and 
CookieHandler.get 
, 
and curiously it looks like they're only used in sun classes in the JDK. So 
this change seems safe to me, unless I've missed something?
While we're looking at CookieManager.java, please fix this typo 
"NetscapeCookieSotre".


  84  * by a more sophisticated CookieStore implementation, e.g. a 
NetscapeCookieSotre.

So, grepcode is kind of hard to work with. I've found that when doing "Find 
Usages" for an API, the results are relative to the JDK version you're looking 
at. Apparently at the time grepcode's indexes were run, most of those other 
libraries weren't building against any version of JDK 8. But if you start the 
search at (say) JDK6 b27, and then go to java.net.CookieHandler.get() and do 
"Find usages", you get a bunch of hits:


http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29=u

Same with 6-b27 CookieManager.get():

http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29=u

7u40-b43 CookieHandler.get():

http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29=u

7u40-b43 CookieManager.get():

http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29=u

I don't know a way to search for the usage of an API across all versions of the 
JDK. It's moderately painful to have to do the same searches for several 
different JDK versions.


Anyway, I spent some time looking through usage hits from the links above. It's 
hard to know how many there are; you just keep hitting "Next" until there aren't 
any more. (Is there a way to get statistics instead of just page after page of 
matches? I don't know.) There are lots of duplicates -- different versions of 
the same library -- so you can skip past those pretty quickly.


After a while some patterns begin to emerge. For this API, the Map that's 
returned is almost always iterated over looking for "Cookie" and "Cookie2" 
entries, or being probed directly for those entries, and is then thrown away. In 
one case it was stored in a private field of a non-serializable class, but the 
only usage of that field was to iterate over it in a manner similar to other cases.


I encourage you to look through these usages as well.

In any case, it's fairly clear to me that the most common use of the returned 
Map is to read stuff out of it immediately. This isn't definitive, obviously, 
but it does look like this is the most common usage pattern. Based on this I 
think the serialization difference won't be a problem, and that it's OK to 
proceed with this change.
3) In my local copy of jdk9, I've removed the TOCTOU comment in 
FileTreeIterator and changed List.of back to Arrays.asList, as your 
explanation regarding FileTreeWalker has convinced me that TOCTOU is not a 
real problem here

OK
4) The 'resolverFields' problem/comment was regarding DateTimeFormatter (see 
this part of latest webrev 
), 
where I realised I couldn't use Set.of instead of 
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I 
noticed that one of the java.time tests was testing whether 
DateTimeFormatter.withResolverFields(TemporalField...) could accept null 
parameters, which made using Set.of impossible since it's null-hostile (as in 
it throws NullPointerException upon being constructed with null element(s)).
Hm, yes, it's odd that there's a test for an array containing a null, in 
addition to an empty array and a null array. See:


jdk/test/java/time/tck/java/time/format/TCKDateTimeFormatter.java

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

[Jonathan Bluett-Duncan]

Hi Stuart,

Thanks for doing such a thorough review of the parts you've managed to look
at so far.

I see you had a number of questions in your numbered bullet points, so I'll
do my best to answer them below.

1) It actually didn't occur to me that there was a potential TOCTOU problem
in ModuleFinder.compose, despite the fact that I found a potential problem
in FileTreeIterator - it completely missed me, so thank you for finding it!
I'll see if I can put a similar comment there to what I wrote in
FileTreeIterator.

2) I seem to remember doing an at least semi-thorough search of the JDK
codebase, and coming to the conclusion that none of the code I touched was
being serialized by the JDK itself. I think I mentioned this is a previous
email, but I also remember saying that I wasn't sure if I took everything
into account, as I'm not that familiar with serialization. So I decided
just now to do a search on Grepcode for usages of CookieManager.get

and
CookieHandler.get
,
and curiously it looks like they're only used in sun classes in the JDK. So
this change seems safe to me, unless I've missed something?

Regarding code search engines, I know of sourcegraph.com, but I think it
requires an account to use their service, and I don't know which
repositories they index apart from GitHub.

3) In my local copy of jdk9, I've removed the TOCTOU comment in
FileTreeIterator and changed List.of back to Arrays.asList, as your
explanation regarding FileTreeWalker has convinced me that TOCTOU is not a
real problem here, and I don't know if future memory improvements to
List.of(T...) (like returning an ImmutableCollections.List1 when there's
only 1 element) are worth the extra time spent copying into a new list.

4) The 'resolverFields' problem/comment was regarding DateTimeFormatter
(see this part of latest webrev
),
where I realised I couldn't use Set.of instead of
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I
noticed that one of the java.time tests was testing whether
DateTimeFormatter.withResolverFields(TemporalField...) could accept null
parameters, which made using Set.of impossible since it's null-hostile (as
in it throws NullPointerException upon being constructed with null
element(s)).

So I never did submit a change with that bit of code replaced with Set.of.
Instead I wrote a comment in the method explaining why one can't use
Set.of. I don't really know if Stephen Colebourne saw that comment, though.

Stephen Colebourne, are you still fine with all the changes I've made to
the java.time classes? http://cr.openjdk.java.net/~reinhapa/reviews/8134373/
webrev.01/

Kind regards,
Jonathan

P.S. I'll wait until everything's reviewed before asking for a new webrev.

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

[Stuart Marks]

Hi Jonathan, all,

I've started to look at this changeset. I'm looking at the one Patrick Reinhart 
posted a couple weeks ago (! sorry):


http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01/

I looked at only a few files and I'm already starting to have questions. Not 
because anybody did anything wrong, but because there are some subtle issues 
that hadn't been raised.


1) ModuleFinder.java

 static ModuleFinder compose(ModuleFinder... finders) {
- final List finderList = Arrays.asList(finders);
- finderList.forEach(Objects::requireNonNull);
+ final List finderList = List.of(finders);

 return new ModuleFinder() { ...

It's important to note that Arrays.asList() wraps a list around an array, 
whereas List.of() has copying semantics. Initially I thought that copying here 
is unnecessary, but after further analysis I've come to believe it's necessary. 
The compose() method is public in the ModuleFinder interface, and the list is 
captured by the ModuleFinder anonymous inner class below, which is returned to 
the caller. Therefore, if Arrays.asList() is used, the caller can mutate the 
array argument and cause the ModuleFinder instance to misbehave. This is a 
TOCTOU problem. Oddly, TOCTOU wasn't discussed with respect to this change, 
though it was with another.


In any case switching to the copying semantics of List.of() in this case is 
correct and prevents TOCTOU problems.


2) CookieManager.java

The old code created a HashMap and wrapped it in a Collections.unmodifiableMap() 
and returned it to the caller. The new code returns some instance created by 
Map.of(). This is a public API. While both versions conform to the specification 
(it says an "immutable" Map is returned) certain behavioral differences are 
visible. In particular, the different objects will serialize differently. It's a 
Map, and it looks like the values are instances of 
ArrayList, so the whole thing will be serializable. It's conceivable that 
applications might take this thing and serialize it. This potentially exposes 
applications to serial interoperability problems if they wanted to exchange 
serial data containing this object between Java 8 and 9.


My hunch is that this problem is fairly unlikely to occur, but I think it would 
be good idea to do some searching to see how often this API is used. If it's 
used rarely, or the typical usages don't involve storing the result somewhere 
(e.g., iterating over the values), then we can proceed.


I use grepcode.com for code searches. I'd be interested in hearing about 
alternative code search engines as well.


3) FileTreeIterator.java

Here's the one where a comment was added describing the copying semantics of 
List.of in order to prevent TOCTOU problems. The options array is passed from 
the outside and thus can be mutated by the caller. But the only time it's used 
is within FileTreeWalker, and there it's iterated over once and never used 
again. This differs from case (1) above where (in the old code) the array 
reference is captured by a long-lived object.


Anyway, in this case, I don't think the copying is necessary, so the old 
Arrays.asList() code is probably fine.


4) Not a specific change, but I saw mention upthread of some change that caused 
one of the java.time tests to fail, something about 'resolverFields' containing 
null. Which change was this, and what's its status in the current webrev?


I note that Stephen Colebourne said that the changes seem "reasonable" but I 
don't know if he saw the mention of the java.time test failure, or whether the 
changeset he reviewed included a change that would have caused it.


* * *

I still need to look at the other changes. If we end up getting hung up on one 
or two, it might make sense to break up the changeset and push part of it. It 
might also make sense to split out the java.time changes, since they're a 
logical chunk, and they potentially have different reviewers. But no need to do 
anything on that just yet.


s'marks




On 9/15/16 12:36 PM, Jonathan Bluett-Duncan wrote:

Hi all,

Stuart, thank you very much for your thorough response.

Regarding serializability concerns, I've just checked my changes and all 
non-test code in the JDK which calls it, and it doesn't seem to me that they 
affect any fields in `Serializable` classes or the return values of methods 
which either return instances of `Serializable` classes or whose javadoc 
mention that the returned object is serializable. So I'm somewhat certain that 
my changes are serialization-compatible, but only somewhat, because I'm not 
that familiar with the intricacies of serialization...


Considering how busy Stuart is, would anyone else be happy to sponsor this 
change?

Kind regards,
Jonathan

On 15 September 2016 at 18:20, Stuart Marks > wrote:


Hi all,

Unfortunately I don't have time to work on any of this at the moment,
because of JavaOne 

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

[Jonathan Bluett-Duncan]

Hi all,

Stuart, thank you very much for your thorough response.

Regarding serializability concerns, I've just checked my changes and all
non-test code in the JDK which calls it, and it doesn't seem to me that
they affect any fields in `Serializable` classes or the return values of
methods which either return instances of `Serializable` classes or whose
javadoc mention that the returned object is serializable. So I'm somewhat
certain that my changes are serialization-compatible, but only somewhat,
because I'm not that familiar with the intricacies of serialization...

Considering how busy Stuart is, would anyone else be happy to sponsor this
change?

Kind regards,
Jonathan

On 15 September 2016 at 18:20, Stuart Marks  wrote:

> Hi all,
>
> Unfortunately I don't have time to work on any of this at the moment,
> because of JavaOne preparation, and JavaOne next week.
>
> Jonathan, thanks for pushing forward with this. I'm glad that others have
> picked it up.
>
> Patrick, thanks for posting the changeset on Jonathan's behalf. This is
> very helpful.
>
> A few comments regarding issues raised up-thread.
>
> Regarding the (non)singleton-ness of the empty collections, this is
> covered by
>
> https://bugs.openjdk.java.net/browse/JDK-8156079
> consider making empty instances singletons
>
> It wasn't a design decision to make them not singletons. The spec
> requirement is only that the returned instance satisfy the requirements of
> the interfaces it implements (e.g., List) and nothing more. Certainly there
> is no spec requirement regarding object identity.
>
> Making the empty collections singletons is the "obvious" thing to do, but
> it's often the case that the "obvious" thing isn't the right thing. That
> said, it may still be the right thing to make them singletons. Given the
> proposed extension to the JDK 9 schedule, it might be possible to change
> this in JDK 9.
>
> Note that List.of() should be functionally equivalent to
> Collections.emptyList() -- and correspondingly for Set and Map -- but they
> do differ. In particular, they have different serialization formats.
>
> Also on this topic, please note comments that Daniel Fuchs and I have
> added to
>
> https://bugs.openjdk.java.net/browse/JDK-8134373
>
> regarding serialization compatibility. Reviewers should take care that
> updating code to use these new collection factories doesn't change any
> serialization formats. Unfortunately I am not confident that we have
> sufficient tests for serialization compatibility.
>
> s'marks
>
>
>
> On 9/15/16 7:02 AM, Jonathan Bluett-Duncan wrote:
>
>> Wow, lots of discussion went on since I was busy doing other stuff!
>>
>> Thanks Patrick for doing the work of creating a new webrev for me. Really
>> appreciated!
>>
>> Pavel already mentioned it, but I think List.of instead of
>> Collections.emptyList in ZoneOffsetTransition is the right thing to do for
>> visual and behavioural consistency. If it turns out that we need to revert
>> to Collections.empty* and Collections.unmodifiable* for e.g.
>> serializability or class loading concerns, then I'd be happy to revert
>> both
>> of the lines I touched. Otherwise I believe that List.of should be used
>> consistently.
>>
>> I think Stuart made List.of() non-singleton because there wasn't any
>> evidence that it made List.of() more memory- or time-intensive than
>> Collections.emptyList(), but I might be wrong on this. I'm sure he can
>> explain more or correct me in this case.
>>
>> Kind regards,
>> Jonathan
>>
>>
>> On 15 September 2016 at 13:33, Patrick Reinhart 
>> wrote:
>>
>> Hello together,
>>>
>>> I tried to process all suggested change input into the following new
>>> webrev:
>>>
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01
>>>
>>> Give me feedback if something is missing/wrong
>>>
>>> -Patrick
>>>
>>>
>>> On 2016-09-15 13:48, Pavel Rappo wrote:
>>>
>>> Daniel, Claes,

 List.of() and Collections.emptyList() are not the same. The behaviours
 are
 different. Moreover, immutable static factory methods return instances
 which are
 value-based. I believe it also means we are not tied with unconditional
 instantiation, and in case of empty collections/maps could probably
 return the
 same object every time.

 We should ask Stuart why it has been done like that in the first place.
 Maybe
 out of concern people might synchronize of those objects? I don't know.
 Let's
 say for now it's an implementation-specific detail.

 On 15 Sep 2016, at 12:35, Claes Redestad 

> wrote:
>
> +1
>
> 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.
>
>
>

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

[Stuart Marks]

Hi all,

Unfortunately I don't have time to work on any of this at the moment, because of 
JavaOne preparation, and JavaOne next week.


Jonathan, thanks for pushing forward with this. I'm glad that others have picked 
it up.


Patrick, thanks for posting the changeset on Jonathan's behalf. This is very 
helpful.


A few comments regarding issues raised up-thread.

Regarding the (non)singleton-ness of the empty collections, this is covered by

https://bugs.openjdk.java.net/browse/JDK-8156079
consider making empty instances singletons

It wasn't a design decision to make them not singletons. The spec requirement is 
only that the returned instance satisfy the requirements of the interfaces it 
implements (e.g., List) and nothing more. Certainly there is no spec requirement 
regarding object identity.


Making the empty collections singletons is the "obvious" thing to do, but it's 
often the case that the "obvious" thing isn't the right thing. That said, it may 
still be the right thing to make them singletons. Given the proposed extension 
to the JDK 9 schedule, it might be possible to change this in JDK 9.


Note that List.of() should be functionally equivalent to Collections.emptyList() 
-- and correspondingly for Set and Map -- but they do differ. In particular, 
they have different serialization formats.


Also on this topic, please note comments that Daniel Fuchs and I have added to

https://bugs.openjdk.java.net/browse/JDK-8134373

regarding serialization compatibility. Reviewers should take care that updating 
code to use these new collection factories doesn't change any serialization 
formats. Unfortunately I am not confident that we have sufficient tests for 
serialization compatibility.


s'marks


On 9/15/16 7:02 AM, Jonathan Bluett-Duncan wrote:

Wow, lots of discussion went on since I was busy doing other stuff!

Thanks Patrick for doing the work of creating a new webrev for me. Really
appreciated!

Pavel already mentioned it, but I think List.of instead of
Collections.emptyList in ZoneOffsetTransition is the right thing to do for
visual and behavioural consistency. If it turns out that we need to revert
to Collections.empty* and Collections.unmodifiable* for e.g.
serializability or class loading concerns, then I'd be happy to revert both
of the lines I touched. Otherwise I believe that List.of should be used
consistently.

I think Stuart made List.of() non-singleton because there wasn't any
evidence that it made List.of() more memory- or time-intensive than
Collections.emptyList(), but I might be wrong on this. I'm sure he can
explain more or correct me in this case.

Kind regards,
Jonathan


On 15 September 2016 at 13:33, Patrick Reinhart  wrote:


Hello together,

I tried to process all suggested change input into the following new
webrev:

http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01

Give me feedback if something is missing/wrong

-Patrick


On 2016-09-15 13:48, Pavel Rappo wrote:


Daniel, Claes,

List.of() and Collections.emptyList() are not the same. The behaviours are
different. Moreover, immutable static factory methods return instances
which are
value-based. I believe it also means we are not tied with unconditional
instantiation, and in case of empty collections/maps could probably
return the
same object every time.

We should ask Stuart why it has been done like that in the first place.
Maybe
out of concern people might synchronize of those objects? I don't know.
Let's
say for now it's an implementation-specific detail.

On 15 Sep 2016, at 12:35, Claes Redestad 

wrote:

+1

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.

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

[Jonathan Bluett-Duncan]

Patrick, would you kindly line-wrap the TOCTOU comment in
http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01/src/java.base/share/classes/java/nio/file/FileTreeIterator.java.cdiff.html
for me, so that each line has 80 characters or less (and maybe insert a `.`
at the end)?

Kind regards,
Jonathan

On 15 September 2016 at 15:06, Jonathan Bluett-Duncan <
jbluettdun...@gmail.com> wrote:

> Pavel,
>
>
>> 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?
>>
>
> When I ran the tests for java.time, one of them failed because `null` was
> being passed to `resolverFields`, so I came the conclusion that it was
> unsafe to make a change there.
>
> Kind regards,
> Jonathan
>
>

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

[Jonathan Bluett-Duncan]

Pavel,


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

When I ran the tests for java.time, one of them failed because `null` was
being passed to `resolverFields`, so I came the conclusion that it was
unsafe to make a change there.

Kind regards,
Jonathan

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

[Jonathan Bluett-Duncan]

Wow, lots of discussion went on since I was busy doing other stuff!

Thanks Patrick for doing the work of creating a new webrev for me. Really
appreciated!

Pavel already mentioned it, but I think List.of instead of
Collections.emptyList in ZoneOffsetTransition is the right thing to do for
visual and behavioural consistency. If it turns out that we need to revert
to Collections.empty* and Collections.unmodifiable* for e.g.
serializability or class loading concerns, then I'd be happy to revert both
of the lines I touched. Otherwise I believe that List.of should be used
consistently.

I think Stuart made List.of() non-singleton because there wasn't any
evidence that it made List.of() more memory- or time-intensive than
Collections.emptyList(), but I might be wrong on this. I'm sure he can
explain more or correct me in this case.

Kind regards,
Jonathan


On 15 September 2016 at 13:33, Patrick Reinhart  wrote:

> Hello together,
>
> I tried to process all suggested change input into the following new
> webrev:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01
>
> Give me feedback if something is missing/wrong
>
> -Patrick
>
>
> On 2016-09-15 13:48, Pavel Rappo wrote:
>
>> Daniel, Claes,
>>
>> List.of() and Collections.emptyList() are not the same. The behaviours are
>> different. Moreover, immutable static factory methods return instances
>> which are
>> value-based. I believe it also means we are not tied with unconditional
>> instantiation, and in case of empty collections/maps could probably
>> return the
>> same object every time.
>>
>> We should ask Stuart why it has been done like that in the first place.
>> Maybe
>> out of concern people might synchronize of those objects? I don't know.
>> Let's
>> say for now it's an implementation-specific detail.
>>
>> On 15 Sep 2016, at 12:35, Claes Redestad 
>>> wrote:
>>>
>>> +1
>>>
>>> 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.
>>>
>>>

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

[Patrick Reinhart]

Hello together,

I tried to process all suggested change input into the following new 
webrev:


http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01

Give me feedback if something is missing/wrong

-Patrick

On 2016-09-15 13:48, Pavel Rappo wrote:

Daniel, Claes,

List.of() and Collections.emptyList() are not the same. The behaviours 
are
different. Moreover, immutable static factory methods return instances 
which are

value-based. I believe it also means we are not tied with unconditional
instantiation, and in case of empty collections/maps could probably 
return the

same object every time.

We should ask Stuart why it has been done like that in the first place. 
Maybe
out of concern people might synchronize of those objects? I don't know. 
Let's

say for now it's an implementation-specific detail.

On 15 Sep 2016, at 12:35, Claes Redestad  
wrote:


+1

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.

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

[Pavel Rappo]

Daniel, Claes,

List.of() and Collections.emptyList() are not the same. The behaviours are
different. Moreover, immutable static factory methods return instances which are
value-based. I believe it also means we are not tied with unconditional
instantiation, and in case of empty collections/maps could probably return the
same object every time.

We should ask Stuart why it has been done like that in the first place. Maybe
out of concern people might synchronize of those objects? I don't know. Let's
say for now it's an implementation-specific detail.

> On 15 Sep 2016, at 12:35, Claes Redestad  wrote:
> 
> +1
> 
> 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. :-)
> 
> /Claes
> 
> On 2016-09-15 13:06, Daniel Fuchs wrote:
>> 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 List 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  
 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, 

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

[Claes Redestad]

+1

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

/Claes

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

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

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

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

[Pavel Rappo]

Daniel, if you're referring to

  388 List getValidOffsets() {
  389 if (isGap()) {
> 390 return List.of();
  391 }
  392 return List.of(getOffsetBefore(), getOffsetAfter());
  393 }

I think in this particular case, List.of is more consistent.

> On 15 Sep 2016, at 12:06, Daniel Fuchs  wrote:
> 
> I find emptyList() more expressive (+ it always returns
> the same instance).

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

[Stephen Colebourne]

The date/time changes seem reasonable.
Stephen

On 14 September 2016 at 21:55, Patrick Reinhart  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 :
>>
>> 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  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
>

Re: 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 List 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  
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  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 :

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

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

[Pavel Rappo]

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

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

[Jonathan Bluett-Duncan]

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