Copilot commented on code in PR #1814:
URL: https://github.com/apache/commons-lang/pull/1814#discussion_r4249832399
##########
src/main/java/org/apache/commons/lang3/AnnotationUtils.java:
##########
@@ -117,6 +117,20 @@ private static boolean annotationArrayMemberEquals(final
Annotation[] a1, final
return true;
}
+ /**
+ * Helper method for generating a hash code for an array of annotations.
+ *
+ * @param annotations The array
+ * @return A hash code for the specified array
+ */
+ private static int annotationArrayMemberHash(final Annotation[]
annotations) {
+ int result = 1;
+ for (final Annotation annotation : annotations) {
+ result = 31 * result + (annotation == null ? 0 :
hashCode(annotation));
+ }
+ return result;
+ }
Review Comment:
`annotationArrayMemberHash` does not match the PR description claim that it
'keeps the element order and seeding of `Arrays.hashCode(Object[])`'.
`Arrays.hashCode(Object[])` uses `result = 31 * result + (element == null ? 0 :
element.hashCode())`, while this method uses
`AnnotationUtils.hashCode(annotation)` for non-null elements. That means the
produced hash is *intentionally different* from `Arrays.hashCode` for
annotation elements with custom `hashCode()`. If the intent is to preserve the
same mixing function but swap the per-element hash, the method is fine—but the
description and/or Javadoc should explicitly state this (e.g., 'mirrors
`Arrays.hashCode` iteration/mixing but uses `AnnotationUtils.hashCode` per
element'). If the goal truly is to match `Arrays.hashCode(Object[])` exactly,
then it should call the element's `hashCode()` instead (which would reintroduce
the original bug), so updating the documentation is the correct fix.
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -526,6 +526,33 @@ void testHashCode() {
});
}
+ @Test
+ void testHashCodeOfNestedAnnotationArray() {
+ assertTimeoutPreemptively(Duration.ofSeconds(666L), () -> {
Review Comment:
Using a 666-second preemptive timeout makes the test suite potentially hang
for a very long time on deadlock/regression, and preemptive timeouts can also
interrupt code under test in a way that masks root causes. Prefer a much
smaller bound for a unit test (typically a few seconds), or remove the timeout
entirely if it's not guarding against a known risk. If a timeout is needed,
align with existing conventions in this test class (e.g., reuse the same
duration used by other tests) to keep suites predictable.
--
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]
