On Mon, Oct 27, 2014 at 1:23 PM, Aleksey Shipilev < aleksey.shipi...@oracle.com> wrote:
> On 10/27/2014 01:53 AM, Peter Levart wrote: > > As you might have noticed, the number of methods obtained from patched > > code differed from original code. I have investigated this and found > > that original code treats abstract class methods the same as abstract > > interface methods as far as multiple inheritance is concerned (it keeps > > them together in the returned array). So I fixed this and here's new > > webrev which behaves the same as original code: > > > > http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/ > > This seems to be a candidate fix for > https://bugs.openjdk.java.net/browse/JDK-8061950, right? If so, please > assign yourself there. It might be advisable to start the separate RFR > thread for a fix? > > My comments for the first reading of the patch: > > * Why MethodList maintains the linked list? Doesn't O(n) > MethodList.add() lead to quadratic complexity again? Should we instead > use the ArrayList<Method> for storing method synonyms? > But N would be the amount of exactly the same overridden methods from superclasses and declared in interfaces. Usually there would be zero or 1-2. So the in-place linked list conserves memory, the extra indirection of ArrayList for n=1 would also be slower. IMO, it's a well designed approach. The only case where it'd actually matter is hundreds of superclasses each overriding all of the their methods. Stanimir > * Please convert some of the indexed loops into for-each loops for > better readability? E.g: > > for (int i = 0; i < intfcsMethods.length; i++) { > for (Method m : intfcsMethods[i]) { > > ...to: > > for (Method[] ms : intfcsMethods) { > for (Method m : ms) { > > * I would think the code would be more readable if MethodList.m was > having a more readable name, e.g. MethodList.base or MethodList.image or > MethodList.primary? > > * Formatting and logic in MethodList.consolidateWith gives me chills. I > think at very least introducing variables for m.getDeclaringClass() and > ml.m.getDeclaringClass() improve readability. Also, moving the comments > at the end of the line should improve readability and better highlight > the boolean expression you are spelling out. Below is the > straight-forward rewrite: > > > MethodList consolidateWith(MethodList ml) { > > Method mineM = m; > > Method otherM = ml.m; > > Class<?> mine = mineM.getDeclaringClass(); > > Class<?> other = otherM.getDeclaringClass(); > > > > if (other == mine // > other is same method as mine > > || !Modifier.isAbstract(mineM.getModifiers()) // > OR, mine is non-abstract method... > > && (other.isAssignableFrom(mine) // > ...which overrides other > > || other.isInterface() && !mine.isInterface())) { // > ...class method which wins over interface > > // just return > > return this; > > } > > if (!Modifier.isAbstract(otherM.getModifiers()) // > other is non-abstract method... > > && (mine.isAssignableFrom(other) // > ...which overrides mine > > || mine.isInterface() && !other.isInterface())) { // > ...class method which wins over interface > > // knock m out and continue > > return (next == null) ? ml : next.consolidateWith(ml); > > } > > > > // else leave m in and continue > > next = (next == null) ? ml : next.consolidateWith(ml); > > return this; > > } > > > * Should we use just methods.put() here? > 2851 MethodList ml0 = methods.putIfAbsent(ml, ml); > > * I wonder if the code would be simpler if we push the Map<MethodList, > MethodList> maintenance to some internal utility class? That would > probably simplify the loops in privateGetPublicMethods? > > Thanks, > -Aleksey. > >
