Default methods for jdk8: request for code review
keith.mcguigan at oracle.com
Mon Oct 22 07:19:18 PDT 2012
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:
> 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:
> 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).
> 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.
> Some of your line reformatting is making things more obscure and
> inconsistent in my view. Eg I find this:
> much clearer than:
> &vtable_size, &num_miranda_methods, &all_mirandas, super_klass(),
> access_flags, class_loader, class_name, local_interfaces,
> 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
> + 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).
> 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.
More information about the lambda-dev