RFR: 8135253: Add push method to CollectionSetChooser

[Mikael Gerdin]

Erik,

On 2015-09-09 15:11, Erik Helin wrote:
> On 2015-09-09, Mikael Gerdin wrote:
>> Hi Erik,
>
> Hi Mikael, thanks for reviewing!
>
>> On 2015-09-09 13:50, Erik Helin wrote:
>>> Hi all,
>>>
>>> this patch adds a new `push` method to CollectionSetChooser so that
>>> regions can be put back at the front of the list. I've split the
>>> patches into three parts to ease reviewing, but I intend to push this as
>>> one commit. While adding the method I also renamed two variables and one
>>> method to make it easier to understand that the CollectionSetChooser
>>> essentially is a stack.
>>>
>>> Part 0: Rename _curr_index to _front and _length to _end:
>>> http://cr.openjdk.java.net/~ehelin/8135253/webrev.00-0/
>>>
>>> Part 1: Rename remove_and_move_to_next() to pop()
>>> http://cr.openjdk.java.net/~ehelin/8135253/webrev.00-1/
>>
>> Given
>> HeapRegion* hr = regions_at(_front);
>> the assert
>> assert(regions_at(_front) == hr, "pre-condition");
>> can probably be removed
>
> Fixed.
>
>> We ususally don't cast unused return values to void.
>> (void) cset_chooser->pop(); // already have region via peek()
>
> Fixed.
>
> Please see new incremental webrev at:
> http://cr.openjdk.java.net/~ehelin/8135253/webrev.01-1/
>
>>>
>>> Part 2: Add the push() method
>>> http://cr.openjdk.java.net/~ehelin/8135253/webrev.00-2/
>>
>> pop does _front += 1, the following seems needlessly verbose:
>>
>>   165   _front = _front - 1;
>
> To make the style similar to the `add_region` method a couple of lines
> above, I changed the expression to `_front--`. Please see new
> incremental webrev at:
> http://cr.openjdk.java.net/~ehelin/8135253/webrev.01-2/
>

Thanks for fixing it!
Looks good.
/Mikael

> Thanks,
> Erik
>
>> /Mikael
>>
>>>
>>> Enhancement:
>>> https://bugs.openjdk.java.net/browse/JDK-8135253
>>>
>>> Testing:
>>> JPRT
>>>
>>> Thanks,
>>> Erik
>>>
>>