You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ParanoidUser (via GitHub)" <gi...@apache.org> on 2023/12/11 17:22:20 UTC
[PR] Address minor redundancies after code inspection [commons-lang]
ParanoidUser opened a new pull request, #1148:
URL: https://github.com/apache/commons-lang/pull/1148
This time the focus is on unnecessary string concatenation and redundant local assignments.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451791220
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
The boxing happens no matter what so it's not unnecessary. Using boxing and unboxing in a level library like this one makes it obvious where to look for possible NPE and possibly unnecessary conversions. IOW, please don't change this code, it's more clear IMO.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454327266
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -539,360 +521,243 @@ public void testIsArrayTypeClasses() {
assertFalse(TypeUtils.isArrayType(String.class));
}
- @SuppressWarnings("boxing") // deliberately used here
@Test
- public void testIsAssignable() throws SecurityException, NoSuchMethodException,
- NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
- final Class<?> clazz = getClass();
- final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
- List.class, List.class, List.class, List.class, List[].class, List[].class,
- List[].class, List[].class, List[].class, List[].class, List[].class);
- final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
- delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
- delegateBooleanAssertion(types, 0, 1, true);
- list0 = list1;
- delegateBooleanAssertion(types, 1, 0, true);
- list2 = list0;
- delegateBooleanAssertion(types, 0, 2, true);
- list0 = list2;
- delegateBooleanAssertion(types, 2, 0, true);
- list3 = list0;
- delegateBooleanAssertion(types, 0, 3, true);
- list0 = list3;
- delegateBooleanAssertion(types, 3, 0, true);
- list4 = list0;
- delegateBooleanAssertion(types, 0, 4, true);
- list0 = list4;
- delegateBooleanAssertion(types, 4, 0, true);
- list5 = list0;
- delegateBooleanAssertion(types, 0, 5, true);
- list0 = list5;
- delegateBooleanAssertion(types, 5, 0, true);
- list6 = list0;
- delegateBooleanAssertion(types, 0, 6, true);
- list0 = list6;
- delegateBooleanAssertion(types, 6, 0, true);
-// list1 = list1;
- delegateBooleanAssertion(types, 1, 1, true);
- list2 = list1;
- delegateBooleanAssertion(types, 1, 2, true);
- list1 = (List<Object>) list2;
- delegateBooleanAssertion(types, 2, 1, false);
- list3 = list1;
- delegateBooleanAssertion(types, 1, 3, true);
- list1 = (List<Object>) list3;
- delegateBooleanAssertion(types, 3, 1, false);
- // list4 = list1;
- delegateBooleanAssertion(types, 1, 4, false);
- // list1 = list4;
- delegateBooleanAssertion(types, 4, 1, false);
- // list5 = list1;
- delegateBooleanAssertion(types, 1, 5, false);
- // list1 = list5;
- delegateBooleanAssertion(types, 5, 1, false);
- list6 = list1;
- delegateBooleanAssertion(types, 1, 6, true);
- list1 = (List<Object>) list6;
- delegateBooleanAssertion(types, 6, 1, false);
-// list2 = list2;
- delegateBooleanAssertion(types, 2, 2, true);
- list2 = list3;
- delegateBooleanAssertion(types, 2, 3, false);
- list2 = list4;
- delegateBooleanAssertion(types, 3, 2, true);
- list3 = (List<? super Object>) list2;
- delegateBooleanAssertion(types, 2, 4, false);
- list2 = list5;
- delegateBooleanAssertion(types, 4, 2, true);
- list4 = (List<String>) list2;
- delegateBooleanAssertion(types, 2, 5, false);
- list2 = list6;
- delegateBooleanAssertion(types, 5, 2, true);
- list5 = (List<? extends String>) list2;
- delegateBooleanAssertion(types, 2, 6, false);
-// list3 = list3;
- delegateBooleanAssertion(types, 6, 2, true);
- list6 = (List<? super String>) list2;
- delegateBooleanAssertion(types, 3, 3, true);
- // list4 = list3;
- delegateBooleanAssertion(types, 3, 4, false);
- // list3 = list4;
- delegateBooleanAssertion(types, 4, 3, false);
- // list5 = list3;
- delegateBooleanAssertion(types, 3, 5, false);
- // list3 = list5;
- delegateBooleanAssertion(types, 5, 3, false);
- list6 = list3;
- delegateBooleanAssertion(types, 3, 6, true);
- list3 = (List<? super Object>) list6;
- delegateBooleanAssertion(types, 6, 3, false);
-// list4 = list4;
- delegateBooleanAssertion(types, 4, 4, true);
- list5 = list4;
- delegateBooleanAssertion(types, 4, 5, true);
- list4 = (List<String>) list5;
- delegateBooleanAssertion(types, 5, 4, false);
- list6 = list4;
- delegateBooleanAssertion(types, 4, 6, true);
- list4 = (List<String>) list6;
- delegateBooleanAssertion(types, 6, 4, false);
-// list5 = list5;
- delegateBooleanAssertion(types, 5, 5, true);
- list6 = (List<? super String>) list5;
- delegateBooleanAssertion(types, 5, 6, false);
- list5 = (List<? extends String>) list6;
- delegateBooleanAssertion(types, 6, 5, false);
-// list6 = list6;
- delegateBooleanAssertion(types, 6, 6, true);
-
-// list7 = list7;
- delegateBooleanAssertion(types, 7, 7, true);
- list8 = list7;
- delegateBooleanAssertion(types, 7, 8, true);
- list7 = list8;
- delegateBooleanAssertion(types, 8, 7, true);
- list9 = list7;
- delegateBooleanAssertion(types, 7, 9, true);
- list7 = list9;
- delegateBooleanAssertion(types, 9, 7, true);
- list10 = list7;
- delegateBooleanAssertion(types, 7, 10, true);
- list7 = list10;
- delegateBooleanAssertion(types, 10, 7, true);
- list11 = list7;
- delegateBooleanAssertion(types, 7, 11, true);
- list7 = list11;
- delegateBooleanAssertion(types, 11, 7, true);
- list12 = list7;
- delegateBooleanAssertion(types, 7, 12, true);
- list7 = list12;
- delegateBooleanAssertion(types, 12, 7, true);
- list13 = list7;
- delegateBooleanAssertion(types, 7, 13, true);
- list7 = list13;
- delegateBooleanAssertion(types, 13, 7, true);
-// list8 = list8;
- delegateBooleanAssertion(types, 8, 8, true);
- list9 = list8;
- delegateBooleanAssertion(types, 8, 9, true);
- list8 = (List<Object>[]) list9;
- delegateBooleanAssertion(types, 9, 8, false);
- list10 = list8;
- delegateBooleanAssertion(types, 8, 10, true);
- list8 = (List<Object>[]) list10; // NOTE cast is required by Sun Java, but not by Eclipse
- delegateBooleanAssertion(types, 10, 8, false);
- // list11 = list8;
- delegateBooleanAssertion(types, 8, 11, false);
- // list8 = list11;
- delegateBooleanAssertion(types, 11, 8, false);
- // list12 = list8;
- delegateBooleanAssertion(types, 8, 12, false);
- // list8 = list12;
- delegateBooleanAssertion(types, 12, 8, false);
- list13 = list8;
- delegateBooleanAssertion(types, 8, 13, true);
- list8 = (List<Object>[]) list13;
- delegateBooleanAssertion(types, 13, 8, false);
-// list9 = list9;
- delegateBooleanAssertion(types, 9, 9, true);
- list10 = (List<? super Object>[]) list9;
- delegateBooleanAssertion(types, 9, 10, false);
- list9 = list10;
- delegateBooleanAssertion(types, 10, 9, true);
- list11 = (List<String>[]) list9;
- delegateBooleanAssertion(types, 9, 11, false);
- list9 = list11;
- delegateBooleanAssertion(types, 11, 9, true);
- list12 = (List<? extends String>[]) list9;
- delegateBooleanAssertion(types, 9, 12, false);
- list9 = list12;
- delegateBooleanAssertion(types, 12, 9, true);
- list13 = (List<? super String>[]) list9;
- delegateBooleanAssertion(types, 9, 13, false);
- list9 = list13;
- delegateBooleanAssertion(types, 13, 9, true);
-// list10 = list10;
- delegateBooleanAssertion(types, 10, 10, true);
- // list11 = list10;
- delegateBooleanAssertion(types, 10, 11, false);
- // list10 = list11;
- delegateBooleanAssertion(types, 11, 10, false);
- // list12 = list10;
- delegateBooleanAssertion(types, 10, 12, false);
- // list10 = list12;
- delegateBooleanAssertion(types, 12, 10, false);
- list13 = list10;
- delegateBooleanAssertion(types, 10, 13, true);
- list10 = (List<? super Object>[]) list13;
- delegateBooleanAssertion(types, 13, 10, false);
-// list11 = list11;
- delegateBooleanAssertion(types, 11, 11, true);
- list12 = list11;
- delegateBooleanAssertion(types, 11, 12, true);
- list11 = (List<String>[]) list12;
- delegateBooleanAssertion(types, 12, 11, false);
- list13 = list11;
- delegateBooleanAssertion(types, 11, 13, true);
- list11 = (List<String>[]) list13;
- delegateBooleanAssertion(types, 13, 11, false);
-// list12 = list12;
- delegateBooleanAssertion(types, 12, 12, true);
- list13 = (List<? super String>[]) list12;
- delegateBooleanAssertion(types, 12, 13, false);
- list12 = (List<? extends String>[]) list13;
- delegateBooleanAssertion(types, 13, 12, false);
-// list13 = list13;
- delegateBooleanAssertion(types, 13, 13, true);
- final Type disType = getClass().getField("dis").getGenericType();
- // Reporter.log( ( ( ParameterizedType ) disType
- // ).getOwnerType().getClass().toString() );
- final Type datType = getClass().getField("dat").getGenericType();
- final Type daType = getClass().getField("da").getGenericType();
- final Type uhderType = getClass().getField("uhder").getGenericType();
- final Type dingType = getClass().getField("ding").getGenericType();
- final Type testerType = getClass().getField("tester").getGenericType();
- final Type tester2Type = getClass().getField("tester2").getGenericType();
- final Type dat2Type = getClass().getField("dat2").getGenericType();
- final Type dat3Type = getClass().getField("dat3").getGenericType();
- dis = dat;
- assertTrue(TypeUtils.isAssignable(datType, disType));
- // dis = da;
- assertFalse(TypeUtils.isAssignable(daType, disType));
- dis = uhder;
- assertTrue(TypeUtils.isAssignable(uhderType, disType));
- dis = ding;
- assertFalse(TypeUtils.isAssignable(dingType, disType),
- String.format("type %s not assignable to %s!", dingType, disType));
- dis = tester;
- assertTrue(TypeUtils.isAssignable(testerType, disType));
- // dis = tester2;
- assertFalse(TypeUtils.isAssignable(tester2Type, disType));
- // dat = dat2;
- assertFalse(TypeUtils.isAssignable(dat2Type, datType));
- // dat2 = dat;
- assertFalse(TypeUtils.isAssignable(datType, dat2Type));
- // dat = dat3;
- assertFalse(TypeUtils.isAssignable(dat3Type, datType));
- final char ch = 0;
- final boolean bo = false;
- final byte by = 0;
- final short sh = 0;
- int in = 0;
- long lo = 0;
- final float fl = 0;
- double du;
- du = ch;
+ public void testIsAssignableClasses() {
assertTrue(TypeUtils.isAssignable(char.class, double.class));
- du = by;
assertTrue(TypeUtils.isAssignable(byte.class, double.class));
- du = sh;
assertTrue(TypeUtils.isAssignable(short.class, double.class));
- du = in;
assertTrue(TypeUtils.isAssignable(int.class, double.class));
- du = lo;
assertTrue(TypeUtils.isAssignable(long.class, double.class));
- du = fl;
assertTrue(TypeUtils.isAssignable(float.class, double.class));
- lo = in;
+
assertTrue(TypeUtils.isAssignable(int.class, long.class));
- lo = Integer.valueOf(0);
assertTrue(TypeUtils.isAssignable(Integer.class, long.class));
- // Long lngW = 1;
assertFalse(TypeUtils.isAssignable(int.class, Long.class));
- // lngW = Integer.valueOf( 0 );
assertFalse(TypeUtils.isAssignable(Integer.class, Long.class));
- in = Integer.valueOf(0);
assertTrue(TypeUtils.isAssignable(Integer.class, int.class));
- final Integer inte = in;
assertTrue(TypeUtils.isAssignable(int.class, Integer.class));
assertTrue(TypeUtils.isAssignable(int.class, Number.class));
assertTrue(TypeUtils.isAssignable(int.class, Object.class));
- final Type intComparableType = getClass().getField("intComparable").getGenericType();
- intComparable = 1;
- assertTrue(TypeUtils.isAssignable(int.class, intComparableType));
assertTrue(TypeUtils.isAssignable(int.class, Comparable.class));
- final Serializable ser = 1;
assertTrue(TypeUtils.isAssignable(int.class, Serializable.class));
- final Type longComparableType = getClass().getField("longComparable").getGenericType();
- // longComparable = 1;
- assertFalse(TypeUtils.isAssignable(int.class, longComparableType));
- // longComparable = Integer.valueOf( 0 );
- assertFalse(TypeUtils.isAssignable(Integer.class, longComparableType));
- // int[] ia;
- // long[] la = ia;
+
assertFalse(TypeUtils.isAssignable(int[].class, long[].class));
- final Integer[] ia = null;
- final Type caType = getClass().getField("intWildcardComparable").getGenericType();
- intWildcardComparable = ia;
- assertTrue(TypeUtils.isAssignable(Integer[].class, caType));
- // int[] ina = ia;
assertFalse(TypeUtils.isAssignable(Integer[].class, int[].class));
- final int[] ina = null;
- final Object[] oa;
- // oa = ina;
assertFalse(TypeUtils.isAssignable(int[].class, Object[].class));
- oa = new Integer[0];
assertTrue(TypeUtils.isAssignable(Integer[].class, Object[].class));
- final Type bClassType = AClass.class.getField("bClass").getGenericType();
- final Type cClassType = AClass.class.getField("cClass").getGenericType();
- final Type dClassType = AClass.class.getField("dClass").getGenericType();
- final Type eClassType = AClass.class.getField("eClass").getGenericType();
- final Type fClassType = AClass.class.getField("fClass").getGenericType();
- final AClass aClass = new AClass(new AAClass<>());
- aClass.bClass = aClass.cClass;
+ }
+
+ @Test
+ public void testIsAssignableGenericComparableTypes() throws NoSuchFieldException {
+ final Type intComparableType = getClass().getField("intComparable").getGenericType();
+ assertTrue(TypeUtils.isAssignable(int.class, intComparableType));
+
+ final Type longComparableType = getClass().getField("longComparable").getGenericType();
+ assertFalse(TypeUtils.isAssignable(int.class, longComparableType));
+ assertFalse(TypeUtils.isAssignable(Integer.class, longComparableType));
+
+ final Type intComparableArrayType = getClass().getField("intWildcardComparable").getGenericType();
+ assertTrue(TypeUtils.isAssignable(Integer[].class, intComparableArrayType));
+ }
+
+ @Test
+ public void testIsAssignableGenericListTypes() throws SecurityException, NoSuchMethodException {
+ final Method method = getClass().getMethod("dummyMethod", List.class, List.class, List.class,
+ List.class, List.class, List.class, List.class, List[].class, List[].class,
+ List[].class, List[].class, List[].class, List[].class, List[].class);
+
+ /*
Review Comment:
I share your sympathy for having clearer type variables. It feels like the `GenericTypeHolder` class might be a better place to store generic types instead of using the `dummyMethod` field.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#issuecomment-1908871747
Is there anything else we need to discuss/address in this PR?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#issuecomment-1908911257
I'm good with this but I'm not a committer so I can't approve the workflow run or merge the pR. @garydgregory
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454350476
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
Agreed. Do you think this rule works for implicit string concatenation as well? Also, should it be project scoped or apply mostly to the code in the `main' folder?
```
String string = "The answer to the universe ";
int number = 42;
return string + number; // implicit
return string + String.valueOf(number); // explicit
```
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451730735
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -539,360 +521,243 @@ public void testIsArrayTypeClasses() {
assertFalse(TypeUtils.isArrayType(String.class));
}
- @SuppressWarnings("boxing") // deliberately used here
@Test
- public void testIsAssignable() throws SecurityException, NoSuchMethodException,
- NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
- final Class<?> clazz = getClass();
- final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
- List.class, List.class, List.class, List.class, List[].class, List[].class,
- List[].class, List[].class, List[].class, List[].class, List[].class);
- final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
- delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
- delegateBooleanAssertion(types, 0, 1, true);
- list0 = list1;
- delegateBooleanAssertion(types, 1, 0, true);
- list2 = list0;
- delegateBooleanAssertion(types, 0, 2, true);
- list0 = list2;
- delegateBooleanAssertion(types, 2, 0, true);
- list3 = list0;
- delegateBooleanAssertion(types, 0, 3, true);
- list0 = list3;
- delegateBooleanAssertion(types, 3, 0, true);
- list4 = list0;
- delegateBooleanAssertion(types, 0, 4, true);
- list0 = list4;
- delegateBooleanAssertion(types, 4, 0, true);
- list5 = list0;
- delegateBooleanAssertion(types, 0, 5, true);
- list0 = list5;
- delegateBooleanAssertion(types, 5, 0, true);
- list6 = list0;
- delegateBooleanAssertion(types, 0, 6, true);
- list0 = list6;
- delegateBooleanAssertion(types, 6, 0, true);
-// list1 = list1;
- delegateBooleanAssertion(types, 1, 1, true);
- list2 = list1;
- delegateBooleanAssertion(types, 1, 2, true);
- list1 = (List<Object>) list2;
- delegateBooleanAssertion(types, 2, 1, false);
- list3 = list1;
- delegateBooleanAssertion(types, 1, 3, true);
- list1 = (List<Object>) list3;
- delegateBooleanAssertion(types, 3, 1, false);
- // list4 = list1;
- delegateBooleanAssertion(types, 1, 4, false);
- // list1 = list4;
- delegateBooleanAssertion(types, 4, 1, false);
- // list5 = list1;
- delegateBooleanAssertion(types, 1, 5, false);
- // list1 = list5;
- delegateBooleanAssertion(types, 5, 1, false);
- list6 = list1;
- delegateBooleanAssertion(types, 1, 6, true);
- list1 = (List<Object>) list6;
- delegateBooleanAssertion(types, 6, 1, false);
-// list2 = list2;
- delegateBooleanAssertion(types, 2, 2, true);
- list2 = list3;
- delegateBooleanAssertion(types, 2, 3, false);
- list2 = list4;
- delegateBooleanAssertion(types, 3, 2, true);
- list3 = (List<? super Object>) list2;
- delegateBooleanAssertion(types, 2, 4, false);
- list2 = list5;
- delegateBooleanAssertion(types, 4, 2, true);
- list4 = (List<String>) list2;
- delegateBooleanAssertion(types, 2, 5, false);
- list2 = list6;
- delegateBooleanAssertion(types, 5, 2, true);
- list5 = (List<? extends String>) list2;
- delegateBooleanAssertion(types, 2, 6, false);
-// list3 = list3;
- delegateBooleanAssertion(types, 6, 2, true);
- list6 = (List<? super String>) list2;
- delegateBooleanAssertion(types, 3, 3, true);
- // list4 = list3;
- delegateBooleanAssertion(types, 3, 4, false);
- // list3 = list4;
- delegateBooleanAssertion(types, 4, 3, false);
- // list5 = list3;
- delegateBooleanAssertion(types, 3, 5, false);
- // list3 = list5;
- delegateBooleanAssertion(types, 5, 3, false);
- list6 = list3;
- delegateBooleanAssertion(types, 3, 6, true);
- list3 = (List<? super Object>) list6;
- delegateBooleanAssertion(types, 6, 3, false);
-// list4 = list4;
- delegateBooleanAssertion(types, 4, 4, true);
- list5 = list4;
- delegateBooleanAssertion(types, 4, 5, true);
- list4 = (List<String>) list5;
- delegateBooleanAssertion(types, 5, 4, false);
- list6 = list4;
- delegateBooleanAssertion(types, 4, 6, true);
- list4 = (List<String>) list6;
- delegateBooleanAssertion(types, 6, 4, false);
-// list5 = list5;
- delegateBooleanAssertion(types, 5, 5, true);
- list6 = (List<? super String>) list5;
- delegateBooleanAssertion(types, 5, 6, false);
- list5 = (List<? extends String>) list6;
- delegateBooleanAssertion(types, 6, 5, false);
-// list6 = list6;
- delegateBooleanAssertion(types, 6, 6, true);
-
-// list7 = list7;
- delegateBooleanAssertion(types, 7, 7, true);
- list8 = list7;
- delegateBooleanAssertion(types, 7, 8, true);
- list7 = list8;
- delegateBooleanAssertion(types, 8, 7, true);
- list9 = list7;
- delegateBooleanAssertion(types, 7, 9, true);
- list7 = list9;
- delegateBooleanAssertion(types, 9, 7, true);
- list10 = list7;
- delegateBooleanAssertion(types, 7, 10, true);
- list7 = list10;
- delegateBooleanAssertion(types, 10, 7, true);
- list11 = list7;
- delegateBooleanAssertion(types, 7, 11, true);
- list7 = list11;
- delegateBooleanAssertion(types, 11, 7, true);
- list12 = list7;
- delegateBooleanAssertion(types, 7, 12, true);
- list7 = list12;
- delegateBooleanAssertion(types, 12, 7, true);
- list13 = list7;
- delegateBooleanAssertion(types, 7, 13, true);
- list7 = list13;
- delegateBooleanAssertion(types, 13, 7, true);
-// list8 = list8;
- delegateBooleanAssertion(types, 8, 8, true);
- list9 = list8;
- delegateBooleanAssertion(types, 8, 9, true);
- list8 = (List<Object>[]) list9;
- delegateBooleanAssertion(types, 9, 8, false);
- list10 = list8;
- delegateBooleanAssertion(types, 8, 10, true);
- list8 = (List<Object>[]) list10; // NOTE cast is required by Sun Java, but not by Eclipse
- delegateBooleanAssertion(types, 10, 8, false);
- // list11 = list8;
- delegateBooleanAssertion(types, 8, 11, false);
- // list8 = list11;
- delegateBooleanAssertion(types, 11, 8, false);
- // list12 = list8;
- delegateBooleanAssertion(types, 8, 12, false);
- // list8 = list12;
- delegateBooleanAssertion(types, 12, 8, false);
- list13 = list8;
- delegateBooleanAssertion(types, 8, 13, true);
- list8 = (List<Object>[]) list13;
- delegateBooleanAssertion(types, 13, 8, false);
-// list9 = list9;
- delegateBooleanAssertion(types, 9, 9, true);
- list10 = (List<? super Object>[]) list9;
- delegateBooleanAssertion(types, 9, 10, false);
- list9 = list10;
- delegateBooleanAssertion(types, 10, 9, true);
- list11 = (List<String>[]) list9;
- delegateBooleanAssertion(types, 9, 11, false);
- list9 = list11;
- delegateBooleanAssertion(types, 11, 9, true);
- list12 = (List<? extends String>[]) list9;
- delegateBooleanAssertion(types, 9, 12, false);
- list9 = list12;
- delegateBooleanAssertion(types, 12, 9, true);
- list13 = (List<? super String>[]) list9;
- delegateBooleanAssertion(types, 9, 13, false);
- list9 = list13;
- delegateBooleanAssertion(types, 13, 9, true);
-// list10 = list10;
- delegateBooleanAssertion(types, 10, 10, true);
- // list11 = list10;
- delegateBooleanAssertion(types, 10, 11, false);
- // list10 = list11;
- delegateBooleanAssertion(types, 11, 10, false);
- // list12 = list10;
- delegateBooleanAssertion(types, 10, 12, false);
- // list10 = list12;
- delegateBooleanAssertion(types, 12, 10, false);
- list13 = list10;
- delegateBooleanAssertion(types, 10, 13, true);
- list10 = (List<? super Object>[]) list13;
- delegateBooleanAssertion(types, 13, 10, false);
-// list11 = list11;
- delegateBooleanAssertion(types, 11, 11, true);
- list12 = list11;
- delegateBooleanAssertion(types, 11, 12, true);
- list11 = (List<String>[]) list12;
- delegateBooleanAssertion(types, 12, 11, false);
- list13 = list11;
- delegateBooleanAssertion(types, 11, 13, true);
- list11 = (List<String>[]) list13;
- delegateBooleanAssertion(types, 13, 11, false);
-// list12 = list12;
- delegateBooleanAssertion(types, 12, 12, true);
- list13 = (List<? super String>[]) list12;
- delegateBooleanAssertion(types, 12, 13, false);
- list12 = (List<? extends String>[]) list13;
- delegateBooleanAssertion(types, 13, 12, false);
-// list13 = list13;
- delegateBooleanAssertion(types, 13, 13, true);
- final Type disType = getClass().getField("dis").getGenericType();
- // Reporter.log( ( ( ParameterizedType ) disType
- // ).getOwnerType().getClass().toString() );
- final Type datType = getClass().getField("dat").getGenericType();
- final Type daType = getClass().getField("da").getGenericType();
- final Type uhderType = getClass().getField("uhder").getGenericType();
- final Type dingType = getClass().getField("ding").getGenericType();
- final Type testerType = getClass().getField("tester").getGenericType();
- final Type tester2Type = getClass().getField("tester2").getGenericType();
- final Type dat2Type = getClass().getField("dat2").getGenericType();
- final Type dat3Type = getClass().getField("dat3").getGenericType();
- dis = dat;
- assertTrue(TypeUtils.isAssignable(datType, disType));
- // dis = da;
- assertFalse(TypeUtils.isAssignable(daType, disType));
- dis = uhder;
- assertTrue(TypeUtils.isAssignable(uhderType, disType));
- dis = ding;
- assertFalse(TypeUtils.isAssignable(dingType, disType),
- String.format("type %s not assignable to %s!", dingType, disType));
- dis = tester;
- assertTrue(TypeUtils.isAssignable(testerType, disType));
- // dis = tester2;
- assertFalse(TypeUtils.isAssignable(tester2Type, disType));
- // dat = dat2;
- assertFalse(TypeUtils.isAssignable(dat2Type, datType));
- // dat2 = dat;
- assertFalse(TypeUtils.isAssignable(datType, dat2Type));
- // dat = dat3;
- assertFalse(TypeUtils.isAssignable(dat3Type, datType));
- final char ch = 0;
- final boolean bo = false;
- final byte by = 0;
- final short sh = 0;
- int in = 0;
- long lo = 0;
- final float fl = 0;
- double du;
- du = ch;
+ public void testIsAssignableClasses() {
assertTrue(TypeUtils.isAssignable(char.class, double.class));
- du = by;
assertTrue(TypeUtils.isAssignable(byte.class, double.class));
- du = sh;
assertTrue(TypeUtils.isAssignable(short.class, double.class));
- du = in;
assertTrue(TypeUtils.isAssignable(int.class, double.class));
- du = lo;
assertTrue(TypeUtils.isAssignable(long.class, double.class));
- du = fl;
assertTrue(TypeUtils.isAssignable(float.class, double.class));
- lo = in;
+
assertTrue(TypeUtils.isAssignable(int.class, long.class));
- lo = Integer.valueOf(0);
assertTrue(TypeUtils.isAssignable(Integer.class, long.class));
- // Long lngW = 1;
assertFalse(TypeUtils.isAssignable(int.class, Long.class));
- // lngW = Integer.valueOf( 0 );
assertFalse(TypeUtils.isAssignable(Integer.class, Long.class));
- in = Integer.valueOf(0);
assertTrue(TypeUtils.isAssignable(Integer.class, int.class));
- final Integer inte = in;
assertTrue(TypeUtils.isAssignable(int.class, Integer.class));
assertTrue(TypeUtils.isAssignable(int.class, Number.class));
assertTrue(TypeUtils.isAssignable(int.class, Object.class));
- final Type intComparableType = getClass().getField("intComparable").getGenericType();
- intComparable = 1;
- assertTrue(TypeUtils.isAssignable(int.class, intComparableType));
assertTrue(TypeUtils.isAssignable(int.class, Comparable.class));
- final Serializable ser = 1;
assertTrue(TypeUtils.isAssignable(int.class, Serializable.class));
- final Type longComparableType = getClass().getField("longComparable").getGenericType();
- // longComparable = 1;
- assertFalse(TypeUtils.isAssignable(int.class, longComparableType));
- // longComparable = Integer.valueOf( 0 );
- assertFalse(TypeUtils.isAssignable(Integer.class, longComparableType));
- // int[] ia;
- // long[] la = ia;
+
assertFalse(TypeUtils.isAssignable(int[].class, long[].class));
- final Integer[] ia = null;
- final Type caType = getClass().getField("intWildcardComparable").getGenericType();
- intWildcardComparable = ia;
- assertTrue(TypeUtils.isAssignable(Integer[].class, caType));
- // int[] ina = ia;
assertFalse(TypeUtils.isAssignable(Integer[].class, int[].class));
- final int[] ina = null;
- final Object[] oa;
- // oa = ina;
assertFalse(TypeUtils.isAssignable(int[].class, Object[].class));
- oa = new Integer[0];
assertTrue(TypeUtils.isAssignable(Integer[].class, Object[].class));
- final Type bClassType = AClass.class.getField("bClass").getGenericType();
- final Type cClassType = AClass.class.getField("cClass").getGenericType();
- final Type dClassType = AClass.class.getField("dClass").getGenericType();
- final Type eClassType = AClass.class.getField("eClass").getGenericType();
- final Type fClassType = AClass.class.getField("fClass").getGenericType();
- final AClass aClass = new AClass(new AAClass<>());
- aClass.bClass = aClass.cClass;
+ }
+
+ @Test
+ public void testIsAssignableGenericComparableTypes() throws NoSuchFieldException {
+ final Type intComparableType = getClass().getField("intComparable").getGenericType();
+ assertTrue(TypeUtils.isAssignable(int.class, intComparableType));
+
+ final Type longComparableType = getClass().getField("longComparable").getGenericType();
+ assertFalse(TypeUtils.isAssignable(int.class, longComparableType));
+ assertFalse(TypeUtils.isAssignable(Integer.class, longComparableType));
+
+ final Type intComparableArrayType = getClass().getField("intWildcardComparable").getGenericType();
+ assertTrue(TypeUtils.isAssignable(Integer[].class, intComparableArrayType));
+ }
+
+ @Test
+ public void testIsAssignableGenericListTypes() throws SecurityException, NoSuchMethodException {
+ final Method method = getClass().getMethod("dummyMethod", List.class, List.class, List.class,
+ List.class, List.class, List.class, List.class, List[].class, List[].class,
+ List[].class, List[].class, List[].class, List[].class, List[].class);
+
+ /*
Review Comment:
It would be even more readable to pull each of these out into an appropriately named local variable and test the variables below.
e.g.
List list = types[0];
List<Object> list_object = types[1]
List<?> list_question_mark = types[2];
etc.
Up to you.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1440381919
##########
src/test/java/org/apache/commons/lang3/math/NumberUtilsTest.java:
##########
@@ -1433,14 +1433,14 @@ public void testStringCreateNumberEnsureNoPrecisionLoss() {
assertTrue(NumberUtils.createNumber("+001.797693134862315759e+308") instanceof BigDecimal);
//LANG-1613
assertTrue(NumberUtils.createNumber(Double.toString(Double.MIN_NORMAL)) instanceof Double);
- assertTrue(NumberUtils.createNumber(Double.toString(Double.MIN_NORMAL) + "D") instanceof Double);
Review Comment:
-1: In this code, a unit test, it is better (IMO) to have a consistent usage pattern that make it obvious we are always working with String objects from the get-go, as opposed to an expression that evaluates to a String. This is because we are testing an API that takes a String as its input.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1455495171
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
IDEs can be configured various ways, but what is it out of the box? I can't say I've ever seen a warning for implicit boxing in a case like the one in this PR.
At the end of the day, implicit boxing is an implementation detail. I prefer the simpler and shorter implicit boxing to making the code longer and harder to read. Implicit boxing is in no way unclear in this method.
There are other improvements that could be make here like inlining FMT_TO_STRING so you don't have to scroll or expand when reading the code. E.g. right now on Github in this PR I can't see what FMT_TO_STRING actually is since Github hides those lines. I have to take extra steps to understand it.
Even better would be to eliminate the format string completely and go back to simple, obvious string concatenation that puts the parts together in the order they're meant to appear instead of mixing all up them. :-)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454350476
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
Agreed. Do you think this rule works for implicit string concatenation as well? Also, should it be project scoped or apply mostly to the code in the `main` folder?
```
String string = "The answer to the universe ";
int number = 42;
return string + number; // implicit
return string + String.valueOf(number); // explicit
```
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451791220
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
The boxing happens no matter what so it's not unnecessary, it's required, implicitly or explicitly. Using boxing and unboxing in a library like this one makes it obvious where to look for possible NPE and possibly unnecessary conversions. IOW, please don't change this code, it's more clear IMO. Also note that you can setup your IDE compiler (like Eclipse) to show you warnings where implicit boxing and unboxing takes place.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451791220
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
The boxing happens no matter what so it's not unnecessary, it's required, implicitly or explicitly. Using boxing and unboxing in a library like this one makes it obvious where to look for possible NPE and possibly unnecessary conversions. IOW, please don't change this code, it's more clear IMO. Also note that you cab setup your IDE compiler (like Eclipse) to show you warnings where implicit boxing and unboxing takes place.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454241745
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
RE boxing we are only going to go in circles because you can also configure an IDE to generate warnings when implicit boxing sites are found (like in Eclipse). Adding comments everywhere this happens is unreasonable IMO.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1448808370
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
final Class<?> clazz = getClass();
final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
List.class, List.class, List.class, List.class, List[].class, List[].class,
List[].class, List[].class, List[].class, List[].class, List[].class);
final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
Review Comment:
This code was originally written very poorly. It's possible it never tested what i was meant to test, though I am curious why certain lines were commented out as if the test would break if they were uncommented.
The delegateBooleanAssertion method should have been inlined, and the entire method should have been broken up into many test methods with one assert per test.
Possibly doing that now might help us understand it better. It might also help if we could figure out how changing the code here by removing one of the assignments causes the test to break. But maybe it doesn't?
It does seem true that all these assignments you've removed seem like they should have no effect.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1448822255
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
final Class<?> clazz = getClass();
final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
List.class, List.class, List.class, List.class, List[].class, List[].class,
List[].class, List[].class, List[].class, List[].class, List[].class);
final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
Review Comment:
Aha! I see what these assignments do. They're pseudo-comments, indicating what the next line is testing. They're bad comments, since they're not actually comments and since they're more confusing than not, but they should be safe to remove.
But wow, is this method bad. The types array is equally confusing. There should be an individual named variable for each type being compared.
If you don't want to go that far, I suggest not deleting these assignments but instead making them comments and maybe expand the comment to something like
// test the generic type of list 1 is assignable to the generic type of list 2
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1449350050
##########
src/test/java/org/apache/commons/lang3/text/ExtendedMessageFormatTest.java:
##########
@@ -415,9 +415,9 @@ public void testExtendedAndBuiltInFormats() {
for (final Locale locale : testLocales) {
final MessageFormat builtins = createMessageFormat(builtinsPattern, locale);
final String expectedPattern = extendedPattern + builtins.toPattern();
- DateFormat df = null;
- NumberFormat nf = null;
- ExtendedMessageFormat emf = null;
+ DateFormat df;
+ NumberFormat nf;
+ ExtendedMessageFormat emf;
if (locale == null) {
Review Comment:
The `testLocales` we're looping through contains the null locale scenario (last value added). So I split this test into the *default locale* scenario and the *available locales* group.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1435424972
##########
src/test/java/org/apache/commons/lang3/RandomStringUtilsTest.java:
##########
@@ -51,7 +51,7 @@ public class RandomStringUtilsTest extends AbstractLangTest {
*/
private double chiSquare(final int[] expected, final int[] observed) {
double sumSq = 0.0d;
- double dev = 0.0d;
+ double dev;
Review Comment:
This one should be declared in the for loop
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
final Class<?> clazz = getClass();
final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
List.class, List.class, List.class, List.class, List[].class, List[].class,
List[].class, List[].class, List[].class, List[].class, List[].class);
final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
Review Comment:
OK, I might see what's going on, in which case these assignments are exactly what's being tested. I'm not sure, but if so they should not be removed
##########
src/test/java/org/apache/commons/lang3/function/MethodFixtures.java:
##########
@@ -124,7 +124,7 @@ public static String staticGetString() {
@AfterEach
public void clear() {
Review Comment:
I don't think this method is needed at all. Whoever wrote it probably had a common misunderstanding about how JUNit works. Every test method gets a completely new object. There's no need to reset fields
##########
src/test/java/org/apache/commons/lang3/RangeTest.java:
##########
@@ -117,20 +117,16 @@ public void testConstructorSignatureWithAbstractComparableClasses() {
final DerivedComparableA derivedComparableA = new DerivedComparableA();
final DerivedComparableB derivedComparableB = new DerivedComparableB();
- Range<AbstractComparable> mixed = Range.between(derivedComparableA, derivedComparableB);
- mixed = Range.between(derivedComparableA, derivedComparableB, null);
+ Range<AbstractComparable> mixed = Range.between(derivedComparableA, derivedComparableB, null);
Review Comment:
This looks OK but I'm not sure what was intended here, but tests code can be weird about things like this. Any chance this was testing that the two initializations in sequence were potentially problematic? I do wish whoever wrote this had left a comment behind explaining their reasoning.
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
Review Comment:
I don't understand at all what rhe original author was doing here. This is weird.
##########
src/test/java/org/apache/commons/lang3/RandomStringUtilsTest.java:
##########
@@ -511,7 +511,7 @@ public void testRandomStringUtils() {
public void testRandomStringUtilsHomog() {
final String set = "abc";
final char[] chars = set.toCharArray();
- String gen = "";
+ String gen;
Review Comment:
Again, if you're doing this move the declaration into the fior loop; don't leave variables undeclared as it's bug prone
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454364042
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
Review Comment:
@garydgregory, it seems that the `GClass` class and field are never used in the tests. Do you think this is a unique case that needs to be covered, or can this class be safely removed?
```suggestion
public class GClass<T extends BClass<? extends T> & AInterface<AInterface<? super T>>> {
// empty
}
...
public GClass gClass;
```
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1448782686
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
final Class<?> clazz = getClass();
final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
List.class, List.class, List.class, List.class, List[].class, List[].class,
List[].class, List[].class, List[].class, List[].class, List[].class);
final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
Review Comment:
I don't really follow the original code here either. To some extent Chesterton's fence is in play. Since I don't understand why somebody built this fence (or code) I'm much more cautious than usual about removing it. :-)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#issuecomment-1884988860
I think we should close this PR unless it gets updated. CC @elharo
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1449247377
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
final Class<?> clazz = getClass();
final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
List.class, List.class, List.class, List.class, List[].class, List[].class,
List[].class, List[].class, List[].class, List[].class, List[].class);
final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
Review Comment:
Wow! Thanks for taking the time to crack this puzzle. Using variable assignments as pseudo-comments is a profoundly original way to confuse people 🙂
Here are a few things I did to make this test a little easier to digest:
- Broke it down into 6 test groups. Let me know if you think we should go with smaller groups;
- Inlined `delegateBooleanAssertion` method;
- Removed the unused private `testIsAssignable` method;
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1448207272
##########
src/test/java/org/apache/commons/lang3/RangeTest.java:
##########
@@ -117,20 +117,16 @@ public void testConstructorSignatureWithAbstractComparableClasses() {
final DerivedComparableA derivedComparableA = new DerivedComparableA();
final DerivedComparableB derivedComparableB = new DerivedComparableB();
- Range<AbstractComparable> mixed = Range.between(derivedComparableA, derivedComparableB);
- mixed = Range.between(derivedComparableA, derivedComparableB, null);
+ Range<AbstractComparable> mixed = Range.between(derivedComparableA, derivedComparableB, null);
Review Comment:
Here is the initial commit (https://github.com/apache/commons-lang/commit/e6eba6e9acbd646e9a28546b14ae1d663dc6751a) for reference. It feels like the intent was to check generic derivatives passed to the method. Sequential initialization is not a problem, but having an extra assertion for 2-arg methods result might be a better way to test this.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451791220
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
The boxing happens no matter what so it's not unnecessary. Using boxing and unboxing in a library like this one makes it obvious where to look for possible NPE and possibly unnecessary conversions. IOW, please don't change this code, it's more clear IMO. Also note that you cab setup your IDE compiler (like Eclipse) to show you warnings where implicit boxing and unboxing takes place.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451791220
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
The boxing happens no matter what so it's not unnecessary. Using boxing and unboxing in a level library like this one makes it obvious where to look for possible NPE and possibly unnecessary conversions. IOW, please don't change this code, it's more clear IMO. Also note that you cab setup your IDE compiler (like Eclipse) to show you warnings where implicit boxing and unboxing takes place.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1451740425
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -539,360 +521,243 @@ public void testIsArrayTypeClasses() {
assertFalse(TypeUtils.isArrayType(String.class));
}
- @SuppressWarnings("boxing") // deliberately used here
@Test
- public void testIsAssignable() throws SecurityException, NoSuchMethodException,
- NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
- final Class<?> clazz = getClass();
- final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
- List.class, List.class, List.class, List.class, List[].class, List[].class,
- List[].class, List[].class, List[].class, List[].class, List[].class);
- final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
- delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
- delegateBooleanAssertion(types, 0, 1, true);
- list0 = list1;
- delegateBooleanAssertion(types, 1, 0, true);
- list2 = list0;
- delegateBooleanAssertion(types, 0, 2, true);
- list0 = list2;
- delegateBooleanAssertion(types, 2, 0, true);
- list3 = list0;
- delegateBooleanAssertion(types, 0, 3, true);
- list0 = list3;
- delegateBooleanAssertion(types, 3, 0, true);
- list4 = list0;
- delegateBooleanAssertion(types, 0, 4, true);
- list0 = list4;
- delegateBooleanAssertion(types, 4, 0, true);
- list5 = list0;
- delegateBooleanAssertion(types, 0, 5, true);
- list0 = list5;
- delegateBooleanAssertion(types, 5, 0, true);
- list6 = list0;
- delegateBooleanAssertion(types, 0, 6, true);
- list0 = list6;
- delegateBooleanAssertion(types, 6, 0, true);
-// list1 = list1;
- delegateBooleanAssertion(types, 1, 1, true);
- list2 = list1;
- delegateBooleanAssertion(types, 1, 2, true);
- list1 = (List<Object>) list2;
- delegateBooleanAssertion(types, 2, 1, false);
- list3 = list1;
- delegateBooleanAssertion(types, 1, 3, true);
- list1 = (List<Object>) list3;
- delegateBooleanAssertion(types, 3, 1, false);
- // list4 = list1;
- delegateBooleanAssertion(types, 1, 4, false);
- // list1 = list4;
- delegateBooleanAssertion(types, 4, 1, false);
- // list5 = list1;
- delegateBooleanAssertion(types, 1, 5, false);
- // list1 = list5;
- delegateBooleanAssertion(types, 5, 1, false);
- list6 = list1;
- delegateBooleanAssertion(types, 1, 6, true);
- list1 = (List<Object>) list6;
- delegateBooleanAssertion(types, 6, 1, false);
-// list2 = list2;
- delegateBooleanAssertion(types, 2, 2, true);
- list2 = list3;
- delegateBooleanAssertion(types, 2, 3, false);
- list2 = list4;
- delegateBooleanAssertion(types, 3, 2, true);
- list3 = (List<? super Object>) list2;
- delegateBooleanAssertion(types, 2, 4, false);
- list2 = list5;
- delegateBooleanAssertion(types, 4, 2, true);
- list4 = (List<String>) list2;
- delegateBooleanAssertion(types, 2, 5, false);
- list2 = list6;
- delegateBooleanAssertion(types, 5, 2, true);
- list5 = (List<? extends String>) list2;
- delegateBooleanAssertion(types, 2, 6, false);
-// list3 = list3;
- delegateBooleanAssertion(types, 6, 2, true);
- list6 = (List<? super String>) list2;
- delegateBooleanAssertion(types, 3, 3, true);
- // list4 = list3;
- delegateBooleanAssertion(types, 3, 4, false);
- // list3 = list4;
- delegateBooleanAssertion(types, 4, 3, false);
- // list5 = list3;
- delegateBooleanAssertion(types, 3, 5, false);
- // list3 = list5;
- delegateBooleanAssertion(types, 5, 3, false);
- list6 = list3;
- delegateBooleanAssertion(types, 3, 6, true);
- list3 = (List<? super Object>) list6;
- delegateBooleanAssertion(types, 6, 3, false);
-// list4 = list4;
- delegateBooleanAssertion(types, 4, 4, true);
- list5 = list4;
- delegateBooleanAssertion(types, 4, 5, true);
- list4 = (List<String>) list5;
- delegateBooleanAssertion(types, 5, 4, false);
- list6 = list4;
- delegateBooleanAssertion(types, 4, 6, true);
- list4 = (List<String>) list6;
- delegateBooleanAssertion(types, 6, 4, false);
-// list5 = list5;
- delegateBooleanAssertion(types, 5, 5, true);
- list6 = (List<? super String>) list5;
- delegateBooleanAssertion(types, 5, 6, false);
- list5 = (List<? extends String>) list6;
- delegateBooleanAssertion(types, 6, 5, false);
-// list6 = list6;
- delegateBooleanAssertion(types, 6, 6, true);
-
-// list7 = list7;
- delegateBooleanAssertion(types, 7, 7, true);
- list8 = list7;
- delegateBooleanAssertion(types, 7, 8, true);
- list7 = list8;
- delegateBooleanAssertion(types, 8, 7, true);
- list9 = list7;
- delegateBooleanAssertion(types, 7, 9, true);
- list7 = list9;
- delegateBooleanAssertion(types, 9, 7, true);
- list10 = list7;
- delegateBooleanAssertion(types, 7, 10, true);
- list7 = list10;
- delegateBooleanAssertion(types, 10, 7, true);
- list11 = list7;
- delegateBooleanAssertion(types, 7, 11, true);
- list7 = list11;
- delegateBooleanAssertion(types, 11, 7, true);
- list12 = list7;
- delegateBooleanAssertion(types, 7, 12, true);
- list7 = list12;
- delegateBooleanAssertion(types, 12, 7, true);
- list13 = list7;
- delegateBooleanAssertion(types, 7, 13, true);
- list7 = list13;
- delegateBooleanAssertion(types, 13, 7, true);
-// list8 = list8;
- delegateBooleanAssertion(types, 8, 8, true);
- list9 = list8;
- delegateBooleanAssertion(types, 8, 9, true);
- list8 = (List<Object>[]) list9;
- delegateBooleanAssertion(types, 9, 8, false);
- list10 = list8;
- delegateBooleanAssertion(types, 8, 10, true);
- list8 = (List<Object>[]) list10; // NOTE cast is required by Sun Java, but not by Eclipse
- delegateBooleanAssertion(types, 10, 8, false);
- // list11 = list8;
- delegateBooleanAssertion(types, 8, 11, false);
- // list8 = list11;
- delegateBooleanAssertion(types, 11, 8, false);
- // list12 = list8;
- delegateBooleanAssertion(types, 8, 12, false);
- // list8 = list12;
- delegateBooleanAssertion(types, 12, 8, false);
- list13 = list8;
- delegateBooleanAssertion(types, 8, 13, true);
- list8 = (List<Object>[]) list13;
- delegateBooleanAssertion(types, 13, 8, false);
-// list9 = list9;
- delegateBooleanAssertion(types, 9, 9, true);
- list10 = (List<? super Object>[]) list9;
- delegateBooleanAssertion(types, 9, 10, false);
- list9 = list10;
- delegateBooleanAssertion(types, 10, 9, true);
- list11 = (List<String>[]) list9;
- delegateBooleanAssertion(types, 9, 11, false);
- list9 = list11;
- delegateBooleanAssertion(types, 11, 9, true);
- list12 = (List<? extends String>[]) list9;
- delegateBooleanAssertion(types, 9, 12, false);
- list9 = list12;
- delegateBooleanAssertion(types, 12, 9, true);
- list13 = (List<? super String>[]) list9;
- delegateBooleanAssertion(types, 9, 13, false);
- list9 = list13;
- delegateBooleanAssertion(types, 13, 9, true);
-// list10 = list10;
- delegateBooleanAssertion(types, 10, 10, true);
- // list11 = list10;
- delegateBooleanAssertion(types, 10, 11, false);
- // list10 = list11;
- delegateBooleanAssertion(types, 11, 10, false);
- // list12 = list10;
- delegateBooleanAssertion(types, 10, 12, false);
- // list10 = list12;
- delegateBooleanAssertion(types, 12, 10, false);
- list13 = list10;
- delegateBooleanAssertion(types, 10, 13, true);
- list10 = (List<? super Object>[]) list13;
- delegateBooleanAssertion(types, 13, 10, false);
-// list11 = list11;
- delegateBooleanAssertion(types, 11, 11, true);
- list12 = list11;
- delegateBooleanAssertion(types, 11, 12, true);
- list11 = (List<String>[]) list12;
- delegateBooleanAssertion(types, 12, 11, false);
- list13 = list11;
- delegateBooleanAssertion(types, 11, 13, true);
- list11 = (List<String>[]) list13;
- delegateBooleanAssertion(types, 13, 11, false);
-// list12 = list12;
- delegateBooleanAssertion(types, 12, 12, true);
- list13 = (List<? super String>[]) list12;
- delegateBooleanAssertion(types, 12, 13, false);
- list12 = (List<? extends String>[]) list13;
- delegateBooleanAssertion(types, 13, 12, false);
-// list13 = list13;
- delegateBooleanAssertion(types, 13, 13, true);
- final Type disType = getClass().getField("dis").getGenericType();
- // Reporter.log( ( ( ParameterizedType ) disType
- // ).getOwnerType().getClass().toString() );
- final Type datType = getClass().getField("dat").getGenericType();
- final Type daType = getClass().getField("da").getGenericType();
- final Type uhderType = getClass().getField("uhder").getGenericType();
- final Type dingType = getClass().getField("ding").getGenericType();
- final Type testerType = getClass().getField("tester").getGenericType();
- final Type tester2Type = getClass().getField("tester2").getGenericType();
- final Type dat2Type = getClass().getField("dat2").getGenericType();
- final Type dat3Type = getClass().getField("dat3").getGenericType();
- dis = dat;
- assertTrue(TypeUtils.isAssignable(datType, disType));
- // dis = da;
- assertFalse(TypeUtils.isAssignable(daType, disType));
- dis = uhder;
- assertTrue(TypeUtils.isAssignable(uhderType, disType));
- dis = ding;
- assertFalse(TypeUtils.isAssignable(dingType, disType),
- String.format("type %s not assignable to %s!", dingType, disType));
- dis = tester;
- assertTrue(TypeUtils.isAssignable(testerType, disType));
- // dis = tester2;
- assertFalse(TypeUtils.isAssignable(tester2Type, disType));
- // dat = dat2;
- assertFalse(TypeUtils.isAssignable(dat2Type, datType));
- // dat2 = dat;
- assertFalse(TypeUtils.isAssignable(datType, dat2Type));
- // dat = dat3;
- assertFalse(TypeUtils.isAssignable(dat3Type, datType));
- final char ch = 0;
- final boolean bo = false;
- final byte by = 0;
- final short sh = 0;
- int in = 0;
- long lo = 0;
- final float fl = 0;
- double du;
- du = ch;
+ public void testIsAssignableClasses() {
assertTrue(TypeUtils.isAssignable(char.class, double.class));
- du = by;
assertTrue(TypeUtils.isAssignable(byte.class, double.class));
- du = sh;
assertTrue(TypeUtils.isAssignable(short.class, double.class));
- du = in;
assertTrue(TypeUtils.isAssignable(int.class, double.class));
- du = lo;
assertTrue(TypeUtils.isAssignable(long.class, double.class));
- du = fl;
assertTrue(TypeUtils.isAssignable(float.class, double.class));
- lo = in;
+
assertTrue(TypeUtils.isAssignable(int.class, long.class));
- lo = Integer.valueOf(0);
assertTrue(TypeUtils.isAssignable(Integer.class, long.class));
- // Long lngW = 1;
assertFalse(TypeUtils.isAssignable(int.class, Long.class));
- // lngW = Integer.valueOf( 0 );
assertFalse(TypeUtils.isAssignable(Integer.class, Long.class));
- in = Integer.valueOf(0);
assertTrue(TypeUtils.isAssignable(Integer.class, int.class));
- final Integer inte = in;
assertTrue(TypeUtils.isAssignable(int.class, Integer.class));
assertTrue(TypeUtils.isAssignable(int.class, Number.class));
assertTrue(TypeUtils.isAssignable(int.class, Object.class));
- final Type intComparableType = getClass().getField("intComparable").getGenericType();
- intComparable = 1;
- assertTrue(TypeUtils.isAssignable(int.class, intComparableType));
assertTrue(TypeUtils.isAssignable(int.class, Comparable.class));
- final Serializable ser = 1;
assertTrue(TypeUtils.isAssignable(int.class, Serializable.class));
- final Type longComparableType = getClass().getField("longComparable").getGenericType();
- // longComparable = 1;
- assertFalse(TypeUtils.isAssignable(int.class, longComparableType));
- // longComparable = Integer.valueOf( 0 );
- assertFalse(TypeUtils.isAssignable(Integer.class, longComparableType));
- // int[] ia;
- // long[] la = ia;
+
assertFalse(TypeUtils.isAssignable(int[].class, long[].class));
- final Integer[] ia = null;
- final Type caType = getClass().getField("intWildcardComparable").getGenericType();
- intWildcardComparable = ia;
- assertTrue(TypeUtils.isAssignable(Integer[].class, caType));
- // int[] ina = ia;
assertFalse(TypeUtils.isAssignable(Integer[].class, int[].class));
- final int[] ina = null;
- final Object[] oa;
- // oa = ina;
assertFalse(TypeUtils.isAssignable(int[].class, Object[].class));
- oa = new Integer[0];
assertTrue(TypeUtils.isAssignable(Integer[].class, Object[].class));
- final Type bClassType = AClass.class.getField("bClass").getGenericType();
- final Type cClassType = AClass.class.getField("cClass").getGenericType();
- final Type dClassType = AClass.class.getField("dClass").getGenericType();
- final Type eClassType = AClass.class.getField("eClass").getGenericType();
- final Type fClassType = AClass.class.getField("fClass").getGenericType();
- final AClass aClass = new AClass(new AAClass<>());
- aClass.bClass = aClass.cClass;
+ }
+
+ @Test
+ public void testIsAssignableGenericComparableTypes() throws NoSuchFieldException {
+ final Type intComparableType = getClass().getField("intComparable").getGenericType();
+ assertTrue(TypeUtils.isAssignable(int.class, intComparableType));
+
+ final Type longComparableType = getClass().getField("longComparable").getGenericType();
+ assertFalse(TypeUtils.isAssignable(int.class, longComparableType));
+ assertFalse(TypeUtils.isAssignable(Integer.class, longComparableType));
+
+ final Type intComparableArrayType = getClass().getField("intWildcardComparable").getGenericType();
+ assertTrue(TypeUtils.isAssignable(Integer[].class, intComparableArrayType));
+ }
+
+ @Test
+ public void testIsAssignableGenericListTypes() throws SecurityException, NoSuchMethodException {
+ final Method method = getClass().getMethod("dummyMethod", List.class, List.class, List.class,
+ List.class, List.class, List.class, List.class, List[].class, List[].class,
+ List[].class, List[].class, List[].class, List[].class, List[].class);
+
+ /*
Review Comment:
Except use camel case for variable names, no underscores.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#issuecomment-1885231763
Hi @garydgregory / @elharo,
My apologies for being slow on this thread for a while. I keep my eyes open for any feedback from you on this particular cleanup task, as well as others like it to come. Before I rush to change the code on the above points, I need to get a little more clarity. I'll add my doubts and intentions later today.
Thank you
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1448272327
##########
src/test/java/org/apache/commons/lang3/RangeTest.java:
##########
@@ -117,20 +117,16 @@ public void testConstructorSignatureWithAbstractComparableClasses() {
final DerivedComparableA derivedComparableA = new DerivedComparableA();
final DerivedComparableB derivedComparableB = new DerivedComparableB();
- Range<AbstractComparable> mixed = Range.between(derivedComparableA, derivedComparableB);
- mixed = Range.between(derivedComparableA, derivedComparableB, null);
+ Range<AbstractComparable> mixed = Range.between(derivedComparableA, derivedComparableB, null);
Review Comment:
For the assertion part, we could use [TypeUtils](https://github.com/apache/commons-lang/blob/master/src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java) to check the range type. Let me know if you have a better idea.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1448251143
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
@@ -543,221 +539,108 @@ public void testIsArrayTypeClasses() {
@Test
public void testIsAssignable() throws SecurityException, NoSuchMethodException,
NoSuchFieldException {
- List list0 = null;
- List<Object> list1;
- List<?> list2;
- List<? super Object> list3;
- List<String> list4;
- List<? extends String> list5;
- List<? super String> list6;
- List[] list7 = null;
- List<Object>[] list8;
- List<?>[] list9;
- List<? super Object>[] list10;
- List<String>[] list11;
- List<? extends String>[] list12;
- List<? super String>[] list13;
final Class<?> clazz = getClass();
final Method method = clazz.getMethod("dummyMethod", List.class, List.class, List.class,
List.class, List.class, List.class, List.class, List[].class, List[].class,
List[].class, List[].class, List[].class, List[].class, List[].class);
final Type[] types = method.getGenericParameterTypes();
-// list0 = list0;
delegateBooleanAssertion(types, 0, 0, true);
- list1 = list0;
Review Comment:
Personally, I don't see how assigning a bunch of unused variables here and there could affect the generic types declared on the dummy method/fields in the test. But since removing these local variables doesn't cause any assertion failures, it feels like (A) they're redundant, and/or (B) we have a broken test that doesn't verify what was intended. Should we try to run it on different JVMs (Oracle/Eclise/etc.)?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1449249499
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,7 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)),
- String.valueOf(getObject()));
+ return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
Review Comment:
Is Integer.valueOf needed here? I think an int might work
##########
src/test/java/org/apache/commons/lang3/text/ExtendedMessageFormatTest.java:
##########
@@ -415,9 +415,9 @@ public void testExtendedAndBuiltInFormats() {
for (final Locale locale : testLocales) {
final MessageFormat builtins = createMessageFormat(builtinsPattern, locale);
final String expectedPattern = extendedPattern + builtins.toPattern();
- DateFormat df = null;
- NumberFormat nf = null;
- ExtendedMessageFormat emf = null;
+ DateFormat df;
+ NumberFormat nf;
+ ExtendedMessageFormat emf;
if (locale == null) {
Review Comment:
locale really shouldn't be null here. If it is, that should be a separate test method.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#issuecomment-1908952364
Building...
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454364042
##########
src/test/java/org/apache/commons/lang3/reflect/TypeUtilsTest.java:
##########
Review Comment:
@garydgregory, it seems that the `GClass` class and field are never used in the tests. Do you think this is a unique case that needs to be covered, or can this class be safely removed?
```
public class GClass<T extends BClass<? extends T> & AInterface<AInterface<? super T>>> {
// empty
}
...
public GClass gClass;
```
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address minor redundancies after code inspection [commons-lang]
Posted by "ParanoidUser (via GitHub)" <gi...@apache.org>.
ParanoidUser commented on code in PR #1148:
URL: https://github.com/apache/commons-lang/pull/1148#discussion_r1454164669
##########
src/main/java/org/apache/commons/lang3/concurrent/ConstantInitializer.java:
##########
@@ -133,6 +133,6 @@ public boolean isInitialized() {
*/
@Override
public String toString() {
- return String.format(FMT_TO_STRING, Integer.valueOf(System.identityHashCode(this)), getObject());
+ return String.format(FMT_TO_STRING, System.identityHashCode(this), getObject());
Review Comment:
I think it was @elharo's suggestion ([quote](https://github.com/apache/commons-lang/pull/1148/commits/1f17f592279d3d9b63ea2fd02be26b62f0656dab#r1449249499)) to remove the explicit integer boxing since it will be automatically boxed either way, so I am not sure which side to take 🙂 .
Since the `System.identityHashCode(this)` method returns a primitive `int` (which is not subject to NPE), I don't see how explicit boxing can help here. Also, by default, IntelliJ IDEA highlights this code with an "unnecessary boxing" warning, which can be annoying and motivate other developers to remove the code. If we decide to leave explicit boxing as is, I'd prefer a comment describing its purpose.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org