demobox commented on a change in pull request #76:
URL: https://github.com/apache/jclouds/pull/76#discussion_r445884200
##########
File path: core/src/main/java/org/jclouds/reflect/Reflection2.java
##########
@@ -153,15 +153,23 @@
.newBuilder().build(new CacheLoader<TypeToken<?>, Set<Invokable<?,
?>>>() {
public Set<Invokable<?, ?>> load(TypeToken<?> key) {
ImmutableSet.Builder<Invokable<?, ?>> builder =
ImmutableSet.<Invokable<?, ?>> builder();
- for (Constructor<?> ctor :
key.getRawType().getDeclaredConstructors()) {
- ctor.setAccessible(true);
+ Class<?> raw = key.getRawType();
+ for (Constructor<?> ctor : raw.getDeclaredConstructors()) {
+ if (!coreJavaClass(raw)) {
+ // In JDK 11 up to 14, the only uses for
`java.beans.ConstructorProperties` annotation
+ // are in the `java.awt`, `java.beans` and `javax.swing`
packages.
+ // Since these constructors are public, there is no need
to call `setAccessible()`
Review comment:
> Instead of doing this, could we just do if (!ctor.isAccessible())
ctor.setAccessible(true)?
@nacx That would indeed be cleaner, but I suspect (as I think @JnRouvignac
is describing) this will not fix the warnings:
* jclouds explicitly tries to make certain non-accessible members accessible
via reflection, and needs this to work
* **some** of those non-accessible members can be made accessible, **some**
trigger warnings
* the latter set contains core classes that jclouds actually does **not**
need to make accessible, so it should be safe to skip them
I no longer recall why we didn't add the "skip core classes" check to
`constructorsForTypeToken` when we added it to `methodsForTypeToken` in db4e4af
(I suspect it wasn't required to fix whatever issue we were seeing then?); it
seems consistent enough to me to add it now, assuming all tests pass.
I agree that your suggestion - only try to make members accessible that
aren't *already* accessible - would be a logic improvement, as we would only
try to modify members where needed. I wonder, though, if it may actually be
less performant, given that `isAccessible` would have to be invoked many times?
I think it makes sense to consider that potential improvement - here and
also in `methodsForTypeToken` - as a separate PR if desired, though.
Happy to discuss further if useful!
----------------------------------------------------------------
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
