Hi Dan,
Thank you for the review.
The new webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.05/
And my answers are in-lined below.
On 12/12/11 09:08 PM, Daniel D. Daugherty wrote:
src/share/vm/services/attachListener.cpp
The new include should be in alpha-order (between lines 36 & 37).
I'm pretty sure that's the new style...
Fixed
Block comment on lines 152-155 should be in '//' style and not
in JavaDoc style (/** ... */)
Fixed
src/share/vm/services/jmm.h
lines 192-208 - HotSpot convention is to mimic the existing style
in a file. For these structs, the field names should all be
lined up (see 181-188 for an example).
Fixed
src/share/vm/services/management.cpp
line 2126 calls JNIHandles::make_local(), but this is a JVM_ENTRY
wrapped function which means it depends on VM_ENTRY_BASE which
has a HandleMarkCleaner in it. Since this is a make_local()
call, won't the local JNIHandle get scrubbed on the way back
to the caller?
Threads have two areas to store handles, one for internal
handles and one for JNI handles. The HandleMarkCleaner
applies only to the internal handles. The purpose of
the make_local() is to create a JNI handle from the
internal handle, so when the internal area is cleaned
up by the HandleMarkCleaner, the JNI handle is still
valid in the context of the caller.
src/share/vm/services/diagnosticArgument.hpp
lines 45-50 - too much indent; should be 4 spaces
Fixed
src/share/vm/services/diagnosticArgument.cpp
HotSpot style is generally 'if (' and not 'if('
Fixed
src/share/vm/services/diagnosticCommand.hpp
lines 28-36 - includes should be in alpha order
Fixed
lines 44, 64 - Parameter defaults in new code is generally frowned
upon in HotSpot. They're used when adding a parameter to existing
code to avoid changing existing calls where the default is OK.
(I happen to disagree with that last part and I think that all
calls should be updated just to make sure your reviewers see
all uses, but I'm just one cranky voice... :-))
I've removed all default parameters I found.
src/share/vm/services/diagnosticCommand.cpp
line 28: includes should be in alpha order
Fixed
lines 31-33 - should be some indent here
Fixed
line 74: It would be useful to display the command that
can't be found:
help foo
Help unavailable: 'foo': No such command
Done.
line 101: just FYI, a ResourceMark without a Thread parameter can
be expensive. Not an issue for HelpDCmd::num_arguments().
I was not aware of the performance penalty. However, this method
is called only once in the lifetime of the VM, so I didn't fixed it.
line 103: HotSpot style is generally 'if (' and not 'if('
Fixed
src/share/vm/services/diagnosticFramework.hpp
line 38: typo: 'provides method' -> 'provides methods'
line 40: typo: 'keywor' -> 'keyword'
Fixed.
lines 43-46 - fields nicely indented here, but not in other new
header files. Any particular reason?
Yes, I got several reviews about the style :-)
lines 48, 154, 218, 286, 324 - Parameter defaults again.
Removed
line 55: is_stop should be:
return !is_empty() && strncmp("stop", _cmd, _cmd_len) == 0;
!strncmp() is frowned upon and spaces between params
OK. Fixed
line 82 - returns a local variable; that shouldn't work.
The method doesn't really return a local variable, this is a
return by value. The return statement invokes the copy constructor
to duplicate the local object into the caller context. Here, the
default copy constructor is called but it's okay because the CmdLine
class has be designed to work this way. I've added a comment in the
code to make this mechanism more explicit.
line 155: missing a space after '='
Fixed
lines 256, 258: HotSpot style is generally 'if (' and not 'if('
Fixed
line 304: typo: 'DCmdFActory' -> 'DCmdFactory'
Fixed
line 306: typo: 'command to be executed' -> 'command from being executed'
Fixed
src/share/vm/services/diagnosticFramework.cpp
line 55: _cmd_len = cmd_end - line;
This length includes any leading white space that was skipped
on lines 42-44. It seems odd that '_cmd' points to the first
non-whitespace in the command, but _cmd_len includes the
leading white space. If you change the _cmd_len calculation,
then you have to also change the logic on line 58 so that
args_len is also not too long.
You're right, this is definitively odd.
I've fixed _cmd_len and updated line 58.
lines 79, 104: typo: 'simple' -> 'single'
Fixed
line 318 - what is 'idx' used for?
Remaining code from a previous version.
Unused now, so I removed it.
line 367: HotSpot style is generally 'if (' and not 'if('
lines 371, 372, 380, 403, 416: space after comma
Fixed
Thank you,
Fred
--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com
