Default methods for jdk8: request for code review

[Keith McGuigan]

Hi David,

Thanks for the feedback!  Comments inline:

On 10/21/2012 10:23 PM, David Holmes wrote:
> Hi Keith,
>
> I'm afraid that is far too extensive a set of changes to provide an
> actual Review on. I've read the design doc and flicked through the code,
> but that's as far as I can go.
>
> One concern I do have:
>
> src/share/vm/utilities/growableArray.hpp
>
> You changed the at(i) access methods to return an object reference
> rather than (I presume) a copy. But is that a safe change to make?

I believe it is (and testing has upheld this so far).  When returning a 
reference (instance of an instance), the copy will be created by the 
caller when the value is returned and assigned.  The only way this could 
cause a problem (I think) is if someone was assigning the result of this 
call to a reference type, which may be illegal since one can't take a 
reference to a temporary.  In any case, we don't use references very 
often in the codebase so this is unlikely to be a problem.

> Other general comments:
>
> src/share/vm/runtime/reflection.cpp
> src/share/vm/classfile/systemDictionary.hpp
> src/share/vm/classfile/vmSymbols.hpp
>
> The changes here doesn't seem to be related to default methods.

Right -- these are changes to support the lambda implementation bundled 
for convenience (and an NPG-292 bugfix that probably isn't needed 
anymore since it's been fixed in the main codebase).

> src/share/vm/oops/method.hpp
>
> Would it be worthwhile defining "MethodType methodType()" directly
> rather than is_overpass() and avoid the bool to MethodType conversion
> when calling allocate() ?

Yes, that's a good idea.  I'll do that, thanks.


> src/share/vm/classfile/classFileParser.cpp
>
> Some of your line reformatting is making things more obscure and
> inconsistent in my view. Eg I find this:
>
>    klassVtable::compute_vtable_size_and_num_mirandas(vtable_size,
>                                                      num_miranda_methods,
>                                                      super_klass(),
>                                                      methods,
>                                                      access_flags,
>                                                      class_loader,
>                                                      class_name,
>                                                      local_interfaces,
>                                                      CHECK_(nullHandle));
>
> much clearer than:
>
>    klassVtable::compute_vtable_size_and_num_mirandas(
>        &vtable_size, &num_miranda_methods, &all_mirandas, super_klass(),
> methods,
>        access_flags, class_loader, class_name, local_interfaces,
>                                                      CHECK_(nullHandle));
>
> Ditto for changes elsewhere eg src/share/vm/oops/klassVtable.cpp

Yeah I can't stand that parameter-passing style and change it whenever I 
can.  The whitespace doesn't add any semantic value or enhance 
documentation at all; it ends up taking up space and making it so you 
can't see as much surrounding context in each screenful.  Plus it 
requires more work for reindentation when there's a change that requires 
it.  Blech.

> ---
>
> src/share/vm/classfile/classFileParser.hpp
>
> +                                 bool* has_default_methods,
> +                                 bool* has_default_method,
>
> why the inconsistency in the plurality of the name?

No good reason.  The first indicates if any of the implemented 
interfaces has any default methods, while the second indicates if 'this' 
class has a default method.  But essentially they're doing the same 
thing -- indicating that the current method has default methods 
somewhere in it's hierarchy (inclusively).

> src/share/vm/oops/instanceKlass.cpp
>
> Interfaces are still stateless so what initialization is required? Are
> these synthetic initialization routines inserted by javac?

Technically we don't need initialization -- what we really need is 
verification (which is done as part of initialization in the 
implementation).  We need to verify the contents of the default methods.

--
- Keith