Default methods for jdk8: request for code review

[Keith McGuigan]

On 10/19/2012 1:23 PM, Coleen Phillimore wrote:
>
> bytecodeAssembler.hpp
>
> Can you add to the top comment about why someone would use this class,
> and why you are using it.  I think if we have other generated methods,
> this is a useful class for people to add to rather than write their own
> in a different way.

No idea why someone else would use this class, but I'll add a comment 
about what I'm doing with it.

> BytecodeCPEntry is a VALUE_OBJ_CLASS_SPEC  (currently)

Fixed.

> defaultMethods.cpp
>
> line 775 - I think this  is the ResourceMark that's keeping everything
> alive right.  I think you should have a comment here that everything is
> resource allocated and not to add ResourceMark's in odd places.

Ok.

> KeepAliveRegistrar - I think this is a good solution to the problem of
> redefining classes while you are processing them.  It usually isn't a
> problem for the class you are finding default methods for because it's
> not in the system dictionary yet.  If a super class is redefined while
> you are processing the methods can it add an implementation for default
> or abstract method?   I think this isn't possible today until extended
> class redefinition.

Right, I don't think that's possible yet, and yes the current class is 
protected from redefinition because it isn't published yet.  So it's 
really the supertypes that this is protecting.

>
> 783 - I think you want a ResourcMark() in here and 657.   These names
> printed might take up enough space for you to worry about running out
> while this is code is executed.

This is debug-only code (the flag is DEVELOP), so I'm less worried about 
running out of memory.  I'd rather keep this code ResourceMark-clean so 
that only the top-level mark is used (see your worry above).

> 647 - if m is an overpass, why do you lookup the method again and why
> can it get a different answer.   A comment why would help because I
> don't know.

I'll add a comment.  The reason is that we're looking for methods which 
would have been mirandas if not for the default method processing on our 
superclass(es).  We can tell which would have been mirandas because they 
are marked as 'is_overpass'.  But that's not quite enough because we 
might have implemented the method ourselves in the current class. 
That's what the second check looks for.

> 81 - seems like super_of() belongs in instanceKlass.hpp - just change
> the type of super() to return InstanceKlass.   Isn't _super always
> InstanceKlass (we can add this as another cleanup, but if you move your
> function we'll get it too in the cleanup).

There is "Klass* InstanceKlass::java_super() const { return super(); }"
I'll try converting that to return an InstanceKlass* instead and see 
what breaks.  If it's not much, I'll just use java_super() in my code. 
Otherwise, I'll add a new method in InstanceKlass.  Maybe 
instance_super()?  Ugly but maybe something like that.

> 48 - a nice comment about what these PseudoScopeMarks are for would be
> useful.   It's so you don't iterate recursively I think.

Well, it's because I don't iterate recursively that I can't use regular 
marks and scope.  I'll add a comment.

> 86 - print_slot and print_method seem like they should be under ifndef
> PRODUCT    And PrintHierarchy should be #ifndef PRODUCT rather than
> ASSERT.   Not that there's a good solid rule around these two
> preprocessor directives, but make sure optimized builds.

Will do.  I suppose I can be more aggressive at cutting out more of this 
other printing code for product builds, too.  Yucky #ifdefs, but oh well...

> I am looking at HiearchyVisitor::run now.  I like how it perfectly fits
> on a page for me.   This looks really good so far.

Cool!  Keep the comments coming.  And thanks!

--
- Keith