RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

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
>>>>
>>
>