paulk-asert commented on PR #2690:
URL: https://github.com/apache/groovy/pull/2690#issuecomment-4956050602
Claude recommends the following
Here's the concrete recommendation for #2690 — a single total, spec-stable
ordering used by both helpers.
## The comparator
```java
/**
* Orders by name, then parameter types, then return type. This is a total
order:
* the JVM forbids two methods in one class sharing a name and descriptor,
and the
* descriptor is exactly the parameter types plus the return type.
*/
private static int compareMethods(final Method m1, final Method m2) {
int c = m1.getName().compareTo(m2.getName());
if (c != 0) return c;
c = compareParameterTypes(m1.getParameterTypes(),
m2.getParameterTypes());
if (c != 0) return c;
return
m1.getReturnType().getName().compareTo(m2.getReturnType().getName());
}
private static int compareParameterTypes(final Class<?>[] p1, final
Class<?>[] p2) {
int c = Integer.compare(p1.length, p2.length);
if (c != 0) return c;
for (int i = 0; i < p1.length; i += 1) {
c = p1[i].getName().compareTo(p2[i].getName());
if (c != 0) return c;
}
return 0;
}
public static Method[] getDeclaredMethodsSorted(final Class<?> type) {
Method[] methods = type.getDeclaredMethods(); // fresh array; safe to
sort in place
Arrays.sort(methods, ReflectionUtils::compareMethods);
return methods;
}
public static Constructor<?>[] getDeclaredConstructorsSorted(final Class<?>
type) {
Constructor<?>[] constructors = type.getDeclaredConstructors();
// name is always <init> and the return type always void, so the
parameters decide
Arrays.sort(constructors, (c1, c2) ->
compareParameterTypes(c1.getParameterTypes(),
c2.getParameterTypes()));
return constructors;
}
```
## Why this shape
**The return type is mandatory, and it is the whole point.** Leave it out
and a bridge method ties with the method it bridges.
**Parameters before return type**, so a bridge sorts adjacent to the method
it bridges and overloads group by signature.
`CachedMethod.compareToCachedMethod` (`CachedMethod.java:126`) orders name →
return type → parameters instead; either is total, and it's worth citing as the
in-repo precedent that *already* includes the return type for exactly this
reason. Pick one and be consistent.
**Explicit keys rather than `Method::toString`.** The PR's current
`thenComparing(Method::toString)` is genuinely correct — `toString()` includes
the return type, so it is total — but its format is unspecified, which means
byte-identical output across JDK versions rests on an implementation detail,
and it rebuilds a long string on every comparison. Comparing class names is
spec-stable and cheaper.
## And drop the fields
Delete `getDeclaredFieldsSorted` and its use in `configureClassNode`.
HotSpot returns declared fields in class-file order — it's the methods array it
keeps sorted by symbol address, which is why methods and constructors vary and
fields don't. So sorting fields buys no determinism, and it *costs*:
`GeneralUtils.getAllProperties` walks `getFields()` in order, so alphabetizing
a precompiled superclass's fields reorders `@TupleConstructor` parameters. It
also makes the reflection path disagree with the decompiler path, which
preserves class-file order.
Constructors, by contrast, live in the methods array as `<init>` and
genuinely do come back in an arbitrary order — so they belong in the fix
alongside methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
