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? * 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.
