for review (XXL): 6655638 method handles for invokedynamic

[Christian Thalinger]

On Thu, 2009-02-12 at 14:41 -0800, John Rose wrote:
> On Feb 12, 2009, at 8:55 AM, Christian Thalinger wrote:
> 
> > On Tue, 2009-01-20 at 02:53 -0800, John Rose wrote:
> >> Please give it a look.
> >
> > I have now reviewed all files.  At least I tried to understand what's
> > happening but methodHandles.{cpp,hpp} is too much, sorry.
> >
> > src/share/vm/classfile/classFileParser.cpp:
> >
> > ClassFileParser::java_dyn_MethodHandle_fix_pre()
> >
> > (*fields_ptr) is used 7 times, readability would be much better if  
> > that
> > value would be assigned to a local variable.
> 
> Yes, I wouldn't have done it that way, except that I'm following the  
> lead of existing code (java_lang_ref_Reference_fix_pre).  Shall we  
> leave it as-is, or fix both?  I could bind a local variable like this:
> 	typeArrayHandle& fields = (*fields_ptr);

I would fix both, but...

> > src/share/vm/prims/methodHandles.cpp:
> >
> > void MethodHandles::set_enabled(bool z) {
> >
> > This method can also disable the method handle, right?  Wouldn't it be
> > clearer if there is a set_enabled() (or enable()) and a set_disabled()
> > (disable()) method (without an argument)?
> 
> This is a recurrent issue with boolean accessors.  Hotspot internal  
> APIs are strongly regular in their use of this pattern for property  
> access:
>    T foo();
>    void set_foo(T x);
> 
> If T is bool, sometimes we have irregularity in the setter name:
>    void set_foo(bool x);
>    void set_is_foo(bool x);
>    void set_has_foo(bool x);
> 
> It is less common to split a boolean setter in two as you suggest:
>    T is_foo();
>    void set_foo();
>    void clear_foo();
> 
> There are cases where the setting is a state transition, as with  
> CompileTask::mark_complete with CT::is_complete.  In such cases, there  
> are two accessor functions, so the split seems natural.
> 
> Some classes have the full three-way pattern, as with  
> JavaThread::is_suspend_equivalent.  That introduces three functions  
> where two will do about as well.
> 
> I guess I prefer not to use the three-way scheme, since it requires a  
> little too much inventiveness in picking positive and negative  
> versions of a property name (e.g., enable vs. disable, set vs.  
> clear).  It makes it harder for non-English-speakers to read the  
> sources, and I'm thinking, of course, of /bin/grep.  :-)
> 
> I prefer the present formulation as simplest and most neutral in  
> connotations.  But if there's a strong reason to change it in this  
> case, I'll do it.

Not that I know of.  I was just proposing.

-- Christian