You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "blackdrag (via GitHub)" <gi...@apache.org> on 2023/12/20 22:20:23 UTC
[PR] Partial cleanup of ReflectionCache [groovy]
blackdrag opened a new pull request, #2018:
URL: https://github.com/apache/groovy/pull/2018
(no comment)
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert closed pull request #2018: Partial cleanup of ReflectionCache
URL: https://github.com/apache/groovy/pull/2018
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on code in PR #2018:
URL: https://github.com/apache/groovy/pull/2018#discussion_r1434337876
##########
src/main/java/org/codehaus/groovy/reflection/CachedClass.java:
##########
@@ -282,14 +274,36 @@ public CachedMethod searchMethods(String name, CachedClass[] parameterTypes) {
CachedMethod res = null;
for (CachedMethod m : methods) {
if (m.getName().equals(name)
- && ReflectionCache.arrayContentsEq(parameterTypes, m.getParameterTypes())
+ && arrayContentsEq(parameterTypes, m.getParameterTypes())
&& (res == null || res.getReturnType().isAssignableFrom(m.getReturnType())))
res = m;
}
return res;
}
+ private static boolean arrayContentsEq(Object[] a1, Object[] a2) {
+ if (a1 == null) {
+ return a2 == null || a2.length == 0;
+ }
+
+ if (a2 == null) {
+ return a1.length == 0;
+ }
+
+ if (a1.length != a2.length) {
+ return false;
+ }
+
+ for (int i = 0; i < a1.length; i++) {
+ if (a1[i] != a2[i]) {
+ return false;
+ }
+ }
+
+ return true;
+ }
Review Comment:
Is there a DGM or JVM method for this now?
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on code in PR #2018:
URL: https://github.com/apache/groovy/pull/2018#discussion_r1434339158
##########
src/main/java/org/codehaus/groovy/reflection/CachedClass.java:
##########
@@ -339,7 +353,7 @@ public String getTypeDescription() {
return BytecodeHelper.getTypeDescription(getTheClass());
}
- public final Class getTheClass() {
+ public final Class<?> getTheClass() {
Review Comment:
I think this is a binary compat issue.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2018:
URL: https://github.com/apache/groovy/pull/2018#discussion_r1434409694
##########
src/main/java/org/codehaus/groovy/reflection/CachedClass.java:
##########
@@ -282,14 +274,36 @@ public CachedMethod searchMethods(String name, CachedClass[] parameterTypes) {
CachedMethod res = null;
for (CachedMethod m : methods) {
if (m.getName().equals(name)
- && ReflectionCache.arrayContentsEq(parameterTypes, m.getParameterTypes())
+ && arrayContentsEq(parameterTypes, m.getParameterTypes())
&& (res == null || res.getReturnType().isAssignableFrom(m.getReturnType())))
res = m;
}
return res;
}
+ private static boolean arrayContentsEq(Object[] a1, Object[] a2) {
+ if (a1 == null) {
+ return a2 == null || a2.length == 0;
+ }
+
+ if (a2 == null) {
+ return a1.length == 0;
+ }
+
+ if (a1.length != a2.length) {
+ return false;
+ }
+
+ for (int i = 0; i < a1.length; i++) {
+ if (a1[i] != a2[i]) {
+ return false;
+ }
+ }
+
+ return true;
+ }
Review Comment:
> Is there a DGM or JVM method for this now?
for line 298+ maybe, though I did not see a DGM for this. There is of course DefaultTypeTransformation#compareArrayEqual, but I would not want to use Array.get if I don't have to. Arrays.equals(a1,a2) could be used for line 294+... It does call equals instead of doing a reference check, but since the Objects in question here are Class instances and Class#equals does also only a reference check it is possible.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2018:
URL: https://github.com/apache/groovy/pull/2018#discussion_r1434931626
##########
src/main/java/org/codehaus/groovy/reflection/CachedClass.java:
##########
@@ -282,14 +274,36 @@ public CachedMethod searchMethods(String name, CachedClass[] parameterTypes) {
CachedMethod res = null;
for (CachedMethod m : methods) {
if (m.getName().equals(name)
- && ReflectionCache.arrayContentsEq(parameterTypes, m.getParameterTypes())
+ && arrayContentsEq(parameterTypes, m.getParameterTypes())
&& (res == null || res.getReturnType().isAssignableFrom(m.getReturnType())))
res = m;
}
return res;
}
+ private static boolean arrayContentsEq(Object[] a1, Object[] a2) {
+ if (a1 == null) {
+ return a2 == null || a2.length == 0;
+ }
+
+ if (a2 == null) {
+ return a1.length == 0;
+ }
+
+ if (a1.length != a2.length) {
+ return false;
+ }
+
+ for (int i = 0; i < a1.length; i++) {
+ if (a1[i] != a2[i]) {
+ return false;
+ }
+ }
+
+ return true;
+ }
Review Comment:
Arrays.equals would be less code to maintain but is likely less efficient. I'm inclined to leave as is for now.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2018:
URL: https://github.com/apache/groovy/pull/2018#discussion_r1434938651
##########
src/main/java/org/codehaus/groovy/reflection/CachedClass.java:
##########
@@ -339,7 +353,7 @@ public String getTypeDescription() {
return BytecodeHelper.getTypeDescription(getTheClass());
}
- public final Class getTheClass() {
+ public final Class<?> getTheClass() {
Review Comment:
I think it is okay in this instance
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2018:
URL: https://github.com/apache/groovy/pull/2018#discussion_r1434399183
##########
src/main/java/org/codehaus/groovy/reflection/CachedClass.java:
##########
@@ -339,7 +353,7 @@ public String getTypeDescription() {
return BytecodeHelper.getTypeDescription(getTheClass());
}
- public final Class getTheClass() {
+ public final Class<?> getTheClass() {
Review Comment:
> I think this is a binary compat issue.
Could you explain how?
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2018:
URL: https://github.com/apache/groovy/pull/2018#issuecomment-1865295756
+1
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Partial cleanup of ReflectionCache [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2018:
URL: https://github.com/apache/groovy/pull/2018#issuecomment-1867578311
Merged - Thanks! I took everything except the import re-ordering for CachedSAMClass. We have until now always just used the default ordering which Intellij uses (original Sun coding style I believe). I backported to Groovy 4, deprecating the relevant methods and carrying over the trivial refactorings.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org