Request for review (XS): JDK-8006430: TraceTypeProfile is a product flag while it should be a diagnostic flag

[Vladimir Kozlov]

I suggested only to add 2 additional lines to the code you are modifying:

strcmp(kind, "{C1 diagnostic}") == 0 ||

strcmp(kind, "{C1 experimental}") == 0 ||

I am not asking you to implement C1_EXPERIMENTAL* and C1_DIAGNOSTIC*.

But may be it is for an other RFE. So please file one and I will agree 
with your current fix.

Thanks,
Vladimir

On 2/8/13 5:52 PM, Krystal Mo wrote:
> Hi Vladimir,
>
> Thank you for the review. I didn't add any C1 related checks because C1
> doesn't have diagnostic/experimental flags declared. If I were to add
> those kinds, I'd touch more files then I'd like.
> Perhaps a new RFE for adding C1 diagnostic/experimental flag kinds?
>
> P.S. Oops, I forgot to update the copyright year. Will be fixed before I
> push.
>
> Thanks,
> Kris
>
> On 02/08/2013 05:36 PM, Vladimir Kozlov wrote:
>> Looks good. I think you can preemptively add "{C1 diagnostic}".
>>
>> Thanks,
>> Vladimir
>>
>> On 2/8/13 5:23 PM, Krystal Mo wrote:
>>> Hi all,
>>>
>>> Could I have a couple of reviews for this fix, please?
>>>
>>> Webrev: http://cr.openjdk.java.net/~kmo/8006430/webrev.00/
>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006430
>>>
>>> This bug is actually not a regression from JDK-7198499, but a long time
>>> issue that C2 diagnostic flags were never treated as being "diagnostic"
>>> since the OpenJDK Mercurial repo history began (duke at 0).
>>>
>>> The problem is, C2 diagnostic flags have the kind "{C2 diagnostic}", but
>>> Flag::is_unlocked() only cared to test "{diagnostic}".
>>> So the fix is straightforward -- find out how many other "diagnostic"
>>> and
>>> "experimental" flag kinds there are, and check them.
>>>
>>> I didn't just check if the string ends with "diagnostic}" or
>>> "experimental}" because I think that's too permissive, but I don't have
>>> a strong opinion on that.
>>> If reviews form a consensus that checking just the postfix is good
>>> enough, I can make the change that way.
>>>
>>> Care has to be taken if new diagnostic/experimental flag kinds are added
>>> to other components in the future, e.g. C1.
>>>
>>> Tested with JPRT and these flags by hand:
>>>    C2 diagnostic: TraceTypeProfile, LoopLimitCheck, RangeLimitCheck
>>>    ARCH diagnostic: UseIncDec
>>>
>>> Thanks,
>>> Kris
>