Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Jonathan Bluett-Duncan]

Okay, that makes sense to me! Thank you for your explanations Claes and
Stuart.

Kind regards,
Jonathan

On 5 October 2016 at 01:57, Stuart Marks  wrote:

> Right, the main point of the comment is to tell the reader the constructor
> isn't superfluous, to prevent it from being cleaned up and accidentally
> causing a regression. Full history can reside in the commit comment, the
> bug database, and in these email logs.
>
> I'd put in a link to a bug only when there's some action on this code
> associated with that bug, e.g., "don't remove this code until bug XXX
> has been fixed."
>
> s'marks
>
>
> On 10/4/16 5:00 PM, Claes Redestad wrote:
>
>> Hi Jonathan,
>>
>> the aim isn't to add an in-depth explanation to the code about exactly
>> the circumstance that led to this constructor and comment being added,
>> but to put a clear message that it was simply, in fact, deliberate, so
>> even the proposed comment might be going further than strictly necessary.
>>
>> I'm also not convinced of the value of putting explicit links to the
>> bug actually pushed, since there's an implicit link in the commit
>> itself anyhow.
>>
>> Thanks!
>>
>> /Claes
>>
>> On 2016-10-04 23:20, Jonathan Bluett-Duncan wrote:
>>
>>> The explanation which Stuart gives for this change in
>>> https://bugs.openjdk.java.net/browse/JDK-8167005 seems to describe why
>>> this constructor is needed much better than the comment itself does. So
>>> I wonder if it's worth adding the link to the bug report in the comment.
>>> E.g.
>>>
>>> // prevent generation of synthetic class required for access to private
>>> // constructor. See: https://bugs.openjdk.java.net/browse/JDK-8167005
>>>
>>> Kind regards,
>>> Jonathan
>>>
>>> On 4 October 2016 at 21:28, Stuart Marks >> > wrote:
>>>
>>>
>>>
>>> On 10/4/16 3:55 AM, Claes Redestad wrote:
>>>
>>>
>>> On 2016-10-04 12:52, Aleksey Shipilev wrote:
>>>
>>> On 10/04/2016 12:50 PM, Claes Redestad wrote:
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~redestad/8167005/webrev.01/
>>> >> >
>>>
>>>
>>> OK.
>>>
>>> Thanks for the speedy review! :-)
>>>
>>>
>>> Thanks for looking at this. The shorter text in the bug report is ok
>>> too.
>>>
>>> s'marks
>>>
>>>
>>>

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Stuart Marks]

Right, the main point of the comment is to tell the reader the constructor isn't 
superfluous, to prevent it from being cleaned up and accidentally causing a 
regression. Full history can reside in the commit comment, the bug database, and 
in these email logs.


I'd put in a link to a bug only when there's some action on this code associated 
with that bug, e.g., "don't remove this code until bug XXX has been fixed."


s'marks

On 10/4/16 5:00 PM, Claes Redestad wrote:

Hi Jonathan,

the aim isn't to add an in-depth explanation to the code about exactly
the circumstance that led to this constructor and comment being added,
but to put a clear message that it was simply, in fact, deliberate, so
even the proposed comment might be going further than strictly necessary.

I'm also not convinced of the value of putting explicit links to the
bug actually pushed, since there's an implicit link in the commit
itself anyhow.

Thanks!

/Claes

On 2016-10-04 23:20, Jonathan Bluett-Duncan wrote:

The explanation which Stuart gives for this change in
https://bugs.openjdk.java.net/browse/JDK-8167005 seems to describe why
this constructor is needed much better than the comment itself does. So
I wonder if it's worth adding the link to the bug report in the comment.
E.g.

// prevent generation of synthetic class required for access to private
// constructor. See: https://bugs.openjdk.java.net/browse/JDK-8167005

Kind regards,
Jonathan

On 4 October 2016 at 21:28, Stuart Marks > wrote:



On 10/4/16 3:55 AM, Claes Redestad wrote:


On 2016-10-04 12:52, Aleksey Shipilev wrote:

On 10/04/2016 12:50 PM, Claes Redestad wrote:

Webrev:
http://cr.openjdk.java.net/~redestad/8167005/webrev.01/



OK.

Thanks for the speedy review! :-)


Thanks for looking at this. The shorter text in the bug report is ok
too.

s'marks

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Claes Redestad]

Hi Jonathan,

the aim isn't to add an in-depth explanation to the code about exactly
the circumstance that led to this constructor and comment being added,
but to put a clear message that it was simply, in fact, deliberate, so
even the proposed comment might be going further than strictly necessary.

I'm also not convinced of the value of putting explicit links to the
bug actually pushed, since there's an implicit link in the commit
itself anyhow.

Thanks!

/Claes

On 2016-10-04 23:20, Jonathan Bluett-Duncan wrote:

The explanation which Stuart gives for this change in
https://bugs.openjdk.java.net/browse/JDK-8167005 seems to describe why
this constructor is needed much better than the comment itself does. So
I wonder if it's worth adding the link to the bug report in the comment.
E.g.

// prevent generation of synthetic class required for access to private
// constructor. See: https://bugs.openjdk.java.net/browse/JDK-8167005

Kind regards,
Jonathan

On 4 October 2016 at 21:28, Stuart Marks > wrote:



On 10/4/16 3:55 AM, Claes Redestad wrote:


On 2016-10-04 12:52, Aleksey Shipilev wrote:

On 10/04/2016 12:50 PM, Claes Redestad wrote:

Webrev:
http://cr.openjdk.java.net/~redestad/8167005/webrev.01/



OK.

Thanks for the speedy review! :-)


Thanks for looking at this. The shorter text in the bug report is ok
too.

s'marks

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Jonathan Bluett-Duncan]

The explanation which Stuart gives for this change in
https://bugs.openjdk.java.net/browse/JDK-8167005 seems to describe why this
constructor is needed much better than the comment itself does. So I wonder
if it's worth adding the link to the bug report in the comment. E.g.

// prevent generation of synthetic class required for access to private
// constructor. See: https://bugs.openjdk.java.net/browse/JDK-8167005

Kind regards,
Jonathan

On 4 October 2016 at 21:28, Stuart Marks  wrote:

>
>
> On 10/4/16 3:55 AM, Claes Redestad wrote:
>
>>
>> On 2016-10-04 12:52, Aleksey Shipilev wrote:
>>
>>> On 10/04/2016 12:50 PM, Claes Redestad wrote:
>>>
 Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/

>>>
>>> OK.
>>>
>>> Thanks for the speedy review! :-)
>>
>
> Thanks for looking at this. The shorter text in the bug report is ok too.
>
> s'marks
>
>

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Stuart Marks]

On 10/4/16 3:55 AM, Claes Redestad wrote:


On 2016-10-04 12:52, Aleksey Shipilev wrote:

On 10/04/2016 12:50 PM, Claes Redestad wrote:

Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/


OK.


Thanks for the speedy review! :-)


Thanks for looking at this. The shorter text in the bug report is ok too.

s'marks

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Chris Hegarty]

> On 4 Oct 2016, at 11:55, Claes Redestad  wrote:
> 
> 
> On 2016-10-04 12:52, Aleksey Shipilev wrote:
>> On 10/04/2016 12:50 PM, Claes Redestad wrote:
>>> Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/

+1

-Chris.

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Claes Redestad]

On 2016-10-04 12:52, Aleksey Shipilev wrote:

On 10/04/2016 12:50 PM, Claes Redestad wrote:

Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/


OK.



Thanks for the speedy review! :-)

/Claes

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Aleksey Shipilev]

On 10/04/2016 12:50 PM, Claes Redestad wrote:
> Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/

OK.

-Aleksey

RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Claes Redestad]

Hi,

as suggested by Stuart Marks in the review thread for JDK-8166840, we
should add a comment as to why an empty constructor is important in the
private inner Itr class, to avoid it being accidentally "cleaned up".

Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8167005

Thanks!

/Claes