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

List:       openjdk-serviceability-dev
Subject:    Re: RFR JDK-8046282: SA update
From:       Poonam Bajaj <poonam.bajaj () oracle ! com>
Date:       2014-06-16 10:36:09
Message-ID: 539EC5C9.2020301 () oracle ! com
[Download RAW message or body]

Thanks for the review, Sundar.

regards,
Poonam


On 6/16/2014 3:54 PM, A. Sundararajan wrote:
> Hi Poonam,
>
> Looks good to me.
>
> -Sundar
>
> On Monday 16 June 2014 02:45 PM, Poonam Bajaj wrote:
>> Hi Sundar,
>>
>> I have updated the enum classes. The updated webrev is available here:
>> http://cr.openjdk.java.net/~poonam/8046282/webrev.01/
>>
>> Please review the changes and let me know your feedback.
>>
>> Thanks,
>> Poonam
>>
>>
>> On 6/12/2014 1:25 PM, Poonam Bajaj wrote:
>>> Hi Sundar,
>>>
>>> Is it okay with you if I keep the enum and class definitions as they 
>>> are now? And can I add you as the reviewer for these changes?
>>>
>>> Thanks,
>>> Poonam
>>>
>>> On 6/9/2014 2:56 PM, A. Sundararajan wrote:
>>>> Since SA is java code, we could have it cleaner..
>>>>
>>>> my 2 cents,
>>>> -Sundar
>>>>
>>>> On Monday 09 June 2014 02:40 PM, Poonam Bajaj wrote:
>>>>> Hi Sundar,
>>>>>
>>>>> Yes, it is possible to do that. e.g.  G1YCType can be defined like 
>>>>> this.
>>>>>
>>>>> public enum G1YCType {
>>>>>   Normal ("Normal"),
>>>>>   InitialMark ("Initial Mark"),
>>>>>   DuringMark ("During Mark"),
>>>>>   Mixed ("Mixed"),
>>>>>   G1YCTypeEndSentinel ("Unknown")
>>>>>
>>>>>   private final String value;
>>>>>
>>>>>   G1YCType(String val) {
>>>>>     this.value = val;
>>>>>   }
>>>>>   public String value() {
>>>>>     return value;
>>>>>   }
>>>>> }
>>>>>
>>>>> But, my purpose of having a class and an enum being used in that 
>>>>> class was to have the similar kind of code structure on the SA 
>>>>> side as is present on the hotspot side. e.g the above is defined 
>>>>> as the following on the hotspot side:
>>>>>
>>>>> 030 enum G1YCType {
>>>>> 031   Normal,
>>>>> 032   InitialMark,
>>>>> 033   DuringMark,
>>>>> 034   Mixed,
>>>>> 035   G1YCTypeEndSentinel
>>>>> 036 };
>>>>> 037
>>>>> 038 class G1YCTypeHelper {
>>>>> 039  public:
>>>>> 040   static const char* to_string(G1YCType type) {
>>>>> 041     switch(type) {
>>>>> 042       case Normal: return "Normal";
>>>>> 043       case InitialMark: return "Initial Mark";
>>>>> 044       case DuringMark: return "During Mark";
>>>>> 045       case Mixed: return "Mixed";
>>>>> 046       default: ShouldNotReachHere(); return NULL;
>>>>> 047     }
>>>>> 048   }
>>>>> 049 };
>>>>>
>>>>> And I have tried to replicate the same on the SA side so that one 
>>>>> can easily understand and map the definitions on both the sides.
>>>>>
>>>>>   27 public class G1YCTypeHelper {
>>>>>   28
>>>>>   29   public enum G1YCType {
>>>>>   30     Normal,
>>>>>   31     InitialMark,
>>>>>   32     DuringMark,
>>>>>   33     Mixed,
>>>>>   34     G1YCTypeEndSentinel
>>>>>   35   }
>>>>>   36
>>>>>   37   public static String toString(G1YCType type) {
>>>>>   38     switch (type) {
>>>>>   39     case Normal:
>>>>>   40       return "Normal";
>>>>>   41     case InitialMark:
>>>>>   42       return "Initial Mark";
>>>>>   43     case DuringMark:
>>>>>   44       return "During Mark";
>>>>>   45     case Mixed:
>>>>>   46       return "Mixed";
>>>>>   47     default:
>>>>>   48       return null;
>>>>>   49     }
>>>>>   50   }
>>>>>   51 }
>>>>>
>>>>>
>>>>> Please let me know if this is still a concern for you.
>>>>>
>>>>> Thanks,
>>>>> Poonam
>>>>>
>>>>> On 6/9/2014 10:38 AM, A. Sundararajan wrote:
>>>>>> The pattern of enum within a class and toString helper to return 
>>>>>> string description of it -- is used for many cases.
>>>>>>
>>>>>> Why not use enums with String accepting constructor and toString 
>>>>>> (or new method called "toDescription()) override? You could have 
>>>>>> add "Unknown" in these enums to map to an option that is 
>>>>>> available in VM -- but not in SA.
>>>>>>
>>>>>> Since it is a debugger it is expected to work with incomplete 
>>>>>> info.. so whereever you map a VM data structure element to java 
>>>>>> enum, you can map unknown enum constants to "Unknown" on Java 
>>>>>> side.  Of course, if there is a sentinel element defined in VM 
>>>>>> side, you could use that itself - no need for "Unknown" in such 
>>>>>> cases.
>>>>>>
>>>>>> It'd nice to simplify that class-within-enum pattern...
>>>>>>
>>>>>> -Sundar
>>>>>>
>>>>>> On Saturday 07 June 2014 02:48 PM, Poonam Bajaj wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review these changes for the bug 8046282 for JDK 9. These 
>>>>>>> changes add some definitions on the SA side and the supporting 
>>>>>>> code on the hotspot side.
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~poonam/8046282/webrev.00/
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8046282
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Poonam
>>>>>>>
>>>>>>
>>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]