[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (S/T): 8231737: Cleanup JvmtiRawMonitor code
From:       David Holmes <david.holmes () oracle ! com>
Date:       2019-10-08 21:25:24
Message-ID: 60d2cfa6-603b-d391-8c25-129af245fc83 () oracle ! com
[Download RAW message or body]

Thanks again Serguei.

David

On 9/10/2019 2:29 am, serguei.spitsyn@oracle.com wrote:
> Looks good.
> 
> Thanks,
> Serguei
> 
> 
> On 10/8/19 06:28, Daniel D. Daugherty wrote:
>> On 10/7/19 9:58 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8231737
>>> webrev: http://cr.openjdk.java.net/~dholmes/8231737/webrev/
>>
>> Thanks for cleaning up this code!!
>>
>> src/hotspot/share/prims/jvmtiRawMonitor.hpp
>>        L58:    QNode* volatile _entry_list;             // Threads blocked on 
>> entry or reentry.
>>        L61:    QNode* volatile _wait_set;                 // Threads wait()ing on 
>> the monitor
>>                Comments no longer line up with their sibling comments.
>>
>>        L62:    volatile jint _waiters;                   // number of waiting threads
>>                Not your bug, but this comment doesn't line up either.
>>
>>        L95:    int raw_wait(jlong millis, bool interruptable, Thread* self);
>>                Not your typo, but: s/interruptable/interruptible/
>>
>> src/hotspot/share/prims/jvmtiRawMonitor.cpp
>>        L133:        node._next   = _entry_list;
>>        L134:        _entry_list   = &node;
>>        L183:    node._t_state       = QNode::TS_WAIT;
>>        L186:    node._next         = _wait_set;
>>        L187:    _wait_set             = &node;
>>                Extra space(s) before the '='.
>>
>>        L346:        --_recursions;
>>        L380:    _waiters++;
>>        L387:    _waiters--;
>>                Not your bug, but inconsistent styles here.
>>                Personally, I prefer post increment and decrement.
>>
>> Thumbs up! I only have nits so feel free to ignore those
>> since you've fixed so many already. I made a pass via frames
>> and a pass via udiffs and tried to spot any accidental
>> changes and didn't find any.
>>
>> Dan
>>
>>
>>>
>>> Stylistic code cleanup of JvmtiRawMonitor code as previously promised:
>>> - Self -> self
>>> - SimpleX -> simpleX
>>> - Contended -> contended
>>> - Node -> node
>>> - TState -> tState (variable name)
>>> - _WaitSet -> _waitSet
>>> - _EntryList -> _entryList;
>>> - All -> all
>>> - remove extra space before ( in function calls
>>> - remove extra space before ;
>>> - remove extra space before ++ and --
>>> - add spaces around binary operators
>>> - use { } on all blocks
>>> - use one statement per line.
>>> - fix indent and alignment (ie remove artificial alignment)
>>>
>>> Probably simplest to look at new code and see if it looks okay rather 
>>> than trying to spot each individual change. :)
>>>
>>> Thanks,
>>> David
>>
> 
[prev in list] [next in list] [prev in thread] [next in thread]