Mark, On 9/16/15 2:11 PM, Mark Thomas wrote: > On 16/09/2015 14:29, Christopher Schultz wrote: >> Mark, >> >> On 9/15/15 5:10 PM, ma...@apache.org wrote: >>> Author: markt >>> Date: Tue Sep 15 21:10:30 2015 >>> New Revision: 1703290 >>> >>> URL: http://svn.apache.org/r1703290 >>> Log: >>> Follow-up to r1703177. >>> Ensure that members never contains an intermediate result of the sorting >>> process. >>> >>> Modified: >>> tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >>> >>> Modified: >>> tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >>> URL: >>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java?rev=1703290&r1=1703289&r2=1703290&view=diff >>> ============================================================================== >>> --- tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >>> (original) >>> +++ tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >>> Tue Sep 15 21:10:30 2015 >>> @@ -66,7 +66,7 @@ public class Membership implements Clone >>> final HashMap<Member, MbrEntry> tmpclone = (HashMap<Member, >>> MbrEntry>) map.clone(); >>> clone.map = tmpclone; >>> clone.members = new Member[members.length]; >>> - System.arraycopy(members,0,clone.members,0,members.length); >>> + System.arraycopy(members, 0, clone.members, 0, members.length); >>> return clone; >>> } >>> } >>> @@ -134,7 +134,12 @@ public class Membership implements Clone >>> >>> updateMember.setMemberAliveTime(member.getMemberAliveTime()); >>> updateMember.setPayload(member.getPayload()); >>> updateMember.setCommand(member.getCommand()); >>> - Arrays.sort(members, memberComparator); >>> + // Re-order. Can't sort in place since a call to >>> + // getMembers() may then receive an intermediate >>> result. >>> + Member[] newMembers = new Member[members.length]; >>> + System.arraycopy(members, 0, newMembers, 0, >>> members.length); >>> + Arrays.sort(newMembers, memberComparator); >>> + members = newMembers; >> >> Consider using Member[].clone here instead of creating your own array >> and manually calling System.arraycopy. Less code and fewer opportunities >> to mass things up. Same performance. > > I'll get this done shortly but I did want to say you should feel free to > make this sort of change directly. I'm happy to do it as a review > comment on one of my patches but I'm not going complain if you want to > go ahead and do this yourself.
Okay. I mostly didn't want to step on your toes if you were doing more refactoring. For the tribes classes, you commented recently that there looked to be a great many improvements to be made in there... I wasn't sure if you had some kind of refactoring in-progress. As for System.arraycopy() -> array[].clone, I also wasn't sure if you were doing it for a reason that wasn't obvious from the diff posted here, or if you had a moral objection to that kind of change :) Oddly enough, array[].clone() is JVM magic: I can't discover it's actual implementation... a debugger steps right over the call just like it would for an assignment. As far as Eclipse is concerned, array[].clone is *not* a method call. -chris
signature.asc
Description: OpenPGP digital signature
