danielsun1106 commented on a change in pull request #905: GROOVY-8339: Fix
warning "An illegal reflective access operation has …
URL: https://github.com/apache/groovy/pull/905#discussion_r275611034
##########
File path:
src/main/java/org/codehaus/groovy/reflection/AccessPermissionChecker.java
##########
@@ -59,13 +60,28 @@ static void checkAccessPermission(Method method) {
}
}
+ static void checkAccessPermission(Constructor constructor) {
+ try {
+ checkAccessPermission(constructor.getDeclaringClass(),
constructor.getModifiers(), constructor.isAccessible());
+ } catch (AccessControlException e) {
+ throw createCacheAccessControlExceptionOf(constructor, e);
+ }
+ }
+
private static CacheAccessControlException
createCacheAccessControlExceptionOf(Method method, AccessControlException e) {
return new CacheAccessControlException(
"Groovy object can not access method " + method.getName()
+ " cacheAccessControlExceptionOf class " +
method.getDeclaringClass().getName()
+ " with modifiers \"" +
Modifier.toString(method.getModifiers()) + "\"", e);
}
+ private static CacheAccessControlException
createCacheAccessControlExceptionOf(Constructor constructor,
AccessControlException e) {
+ return new CacheAccessControlException(
Review comment:
> `CacheAccessControlException` could perhaps also override
`fillInStackTrace()` to avoid filling in the stack trace, which is an expensive
operation?
@glaforge Thanks for your suggestion. We should be able to override the
`fillInStackTrace()`, but I am not sure whether we do not really need the stack
trace information.
```java
/**
* Fills in the execution stack trace. This method records within this
* {@code Throwable} object information about the current state of
* the stack frames for the current thread.
*
* <p>If the stack trace of this {@code Throwable} {@linkplain
* Throwable#Throwable(String, Throwable, boolean, boolean) is not
* writable}, calling this method has no effect.
*
* @return a reference to this {@code Throwable} instance.
* @see java.lang.Throwable#printStackTrace()
*/
public synchronized Throwable fillInStackTrace() {
if (stackTrace != null ||
backtrace != null /* Out of protocol state */ ) {
fillInStackTrace(0);
stackTrace = UNASSIGNED_STACK;
}
return this;
}
```
As you know, `CacheAccessControlException` is an existing exception class,
which has been used in Groovy implementation(shown as follows), so @melix's
worries also exists in the Groovy for a long time ;-)
*
https://github.com/apache/groovy/blob/master/src/main/java/org/codehaus/groovy/reflection/AccessPermissionChecker.java#L63
*
https://github.com/apache/groovy/blob/master/src/main/java/org/codehaus/groovy/reflection/AccessPermissionChecker.java#L78
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
