Mikael,
Sorry for the confusion. I see what you mean.
I missed that you made it consistent with the current style in these files.
Agree, that is the best you can do.
Thanks,
Serguei
On 11/26/12 2:37 PM, Mikael Vidstedt wrote:
Serguei,
I'm confused. When I look at the latest webrev
(http://cr.openjdk.java.net/~mikael/8003879/webrev.02/) I believe the
indentation is correct - correct being defined as aligned on the first
letter of the type name. Please clarify what you expect if you do not
agree.
Thanks,
Mikael
On 11/26/2012 12:21 PM, serguei.spit...@oracle.com wrote:
Looks good.
However, it'd be nice to fix the indentation commented by David (use
frames to notice it):
*vmStructs_cms.hpp*:
68 declare_type(AFLBinaryTreeDictionary,
FreeBlockDictionary<FreeChunk>)
*jni.cpp:*
2110 declare_type(MetablockTreeDictionary,
FreeBlockDictionary<Metablock>)
Thanks,
Serguei
On 11/22/12 5:04 PM, Mikael Vidstedt wrote:
On 2012-11-22 16:35, David Holmes wrote:
Hi Mikael,
I prefer this as an internal test too. Though I don't understand
why execute_internal_vm_tests is in jni.cpp rather than jvm.cpp -
this is an enhancement to the VM interface not the JNI interface
isn't it? (Separate issue of course).
I think we'll find that when we get a few more tests in there it's
time to look at breaking it out into a separate file completely or
do it in a completely different way, but that's for another day. I
do hope it's soon though.
BTW the weird indentation makes more sense viewing frames rather
than diffs :)
Right :)
Looks good to go.
Thanks! Eagerly awaiting a second review!
/Mikael
David
On 23/11/2012 10:16 AM, Mikael Vidstedt wrote:
I gave it some more thought and decided to try what it would look
like
having the test be part of the ExecuteInternalVMTests family
instead. I
like this one better personally, but comments are appreciated!
http://cr.openjdk.java.net/~mikael/8003879/webrev.02
Cheers,
Mikael
On 2012-11-22 14:25, Mikael Vidstedt wrote:
New webrev available here:
http://cr.openjdk.java.net/~mikael/8003879/webrev.01
Cheers,
Mikael
On 2012-11-22 14:19, Mikael Vidstedt wrote:
On 2012-11-21 20:17, David Holmes wrote:
Hi Mikael,
On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:
Please review the below change.
The change for 7045397 introduced a couple of duplicate
entries in the
vmStructs::localHotSpotVMTypes array. This shows up when using
the
jmap
tool in a rather ugly way:
<snip>
Other than an indentation problem (which I don't think you
introduced) this fixup seems fine.
For some reason that's the standard for declaring types in that
long
list - aligning on the first letter of the type name. I followed
suit.
In addition to removing the two duplicated entries I also added a
simple, naive runtime test to walk through and make sure no
type is
repeated. The VMStructs::init only called in debug_only so
there's no
startup overhead in product, but it may be better to turn the
test
into
a unit test and only running it as part of
ExecuteInternalVMTests.
Feedback appreciated!
I have a few comments on this part:
- Array indices should be int's not size_t (ie signed not
unsigned)
Will fix.
- I can't clearly see how the localHotSpotVMTypes array is
declared
or filled but I assume there is guaranteed to be a sentinel
entry at
the end so that we don't index past the end?
Yeah. I'm not sure you want to know. It took me a few minutes to
figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY
macro which generates the end marker is passed to VM_TYPES,
VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is
allowed to
actually allowed to call the macro and actually generate the
marker,
meaning only the "implementation" of VM_TYPES_OS_CPU actually
has the
call to last_entry(). Not sure why the end marker isn't just
explicitly added at the end instead, but I think I'll queue that up
for later.
All in all, as far as I can tell there is one and only one end
marker
and it is generated as part of the expansion of VM_TYPES_OS_CPU.
- assert(0, ...) should be assert(false, ...) (as per style guide
[1] ;-) )
Will fix.
That all said I'm not sure this test belongs there, but I don't
feel
strongly either way.
Me neither :)
Cheers,
Mikael
Cheers,
David
[1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide
http://cr.openjdk.java.net/~mikael/8003879/webrev.00/
Cheers,
Mikael
