On 8/10/2019 11:07 pm, [email protected] wrote:
It is. And it's technically trivial so you can push it since lots of
people have seen it.
And everyone spots something else that needs adjusting :) But I think I
have everything now that Dan has had a look.
Cheers,
David
Coleen
On 10/8/19 9:06 AM, David Holmes wrote:
Thanks Coleen, I'll take this as a review :)
David
On 8/10/2019 10:59 pm, [email protected] wrote:
Thank you!!
On 10/8/19 12:42 AM, David Holmes wrote:
Hi Serguei,
On 8/10/2019 2:18 pm, [email protected] wrote:
Hi David,
Looks good in general.
Thanks for looking at it.
A couple of places still need a cleanup.
http://cr.openjdk.java.net/~dholmes/8231737/webrev/src/hotspot/share/prims/jvmtiRawMonitor.hpp.frames.html
QNode pointer is not unified:
47 QNode * volatile _next;
48 QNode * volatile _prev;
68 void simpleEnter(Thread * self) ;
69 void simpleExit(Thread * self) ;
but:53 QNode(Thread* thread); 56 Thread* volatile _owner; //
pointer to owning thread 58 QNode* volatile _entryList; // Threads
blocked on entry or reentry.
61 QNode* volatile _waitSet; // Threads wait()ing on the
monitor There are more places where a space is placed or not
between the type name and the '*'.
Okay I have switched everything to the:
type* var
style. It seems most prevelant based on a grep for "Thread* t" type
usage.
Yeah, this is the style chosen for the Hotspot sources, because the
star is part of the type, not part of the name. But don't argue with
me about it because I'm repeating the rationale that people who felt
strongly about it gave. It's important to me that it's consistent.
thanks!
Coleen
Missed space before ';':
68 void simpleEnter(Thread * self) ;
69 void simpleExit(Thread * self) ;
70 int simpleWait(Thread * self, jlong millis) ;
71 void simpleNotify(Thread * self, bool all) ;
Fixed.
http://cr.openjdk.java.net/~dholmes/8231737/webrev/src/hotspot/share/prims/jvmtiRawMonitor.cpp.frames.html
Space is not needed after the cast: 280 jt = (JavaThread*) self;
370 jt = (JavaThread*) self;
Other places do not have it:
150 OrderAccess::release_store(&_owner, (Thread*)NULL); 288
contended = Atomic::cmpxchg(jt, &_owner, (Thread*)NULL); 291
contended = Atomic::cmpxchg(self, &_owner, (Thread*)NULL);
Fixed.
webrev updated in place.
Thanks,
David
-----
Thanks,
Serguei
On 10/7/19 18:58, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8231737
webrev: http://cr.openjdk.java.net/~dholmes/8231737/webrev/
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
