review (M) for 6892658: C2 should optimize some stringbuilder patterns

[Tom Rodriguez]

No, there's definitely something wrong.  It worked to match what I wanted to so I didn't look that closely but it will certainly match a broader range of things than it should.  I would expect the match functions to be disjoint from each other but both F_R and F_S ignore the JVM_ACC_NATIVE bit so F_RY matches F_R and F_SN matches F_S though the reverse isn't true.

Anyway, thanks for catching this.  I've fixed match_F_Y as you indicated and updated the flags enum to more clearly report the indeterminate nature of the native bit in some of the types.

    F_none = 0,
    F_R,                        // !static ?native !synchronized (R="regular")                               
    F_S,                        //  static ?native !synchronized                                             
    F_Y,                        // !static ?native  synchronized                                             
    F_RN,                       // !static  native !synchronized                                             
    F_SN,                       //  static  native !synchronized                                             
    F_RNY                       // !static  native  synchronized

I may file a separate bug to add testing of JVM_ACC_NATIVE to the one which are currently ignoring it.

tom

On Nov 10, 2009, at 5:55 AM, Andreas Kohn wrote:

> On Mon, 2009-11-09 at 15:46 -0800, Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6892658/
> Hi,
> 
> more question a question than a review: should match_F_Y() not require
> synchronized?
> 
> 306 inline bool match_F_Y(jshort flags) {
> 307   const int req = 0;
>                     = JVM_ACC_SYNCHRONIZED; ?
> 308   const int neg = JVM_ACC_STATIC;
> 309   return (flags & (req | neg)) == req;
> 310 }
> 
> I'm just a casual reader, so I might be missing something here. 
> 
> Regards,
> --
> Andreas
> 
> -- 
> Never attribute to malice that which can be adequately explained by
> stupidity.                                        -- Hanlon's Razor