-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(shadow): fix the creation of a partial shadow class #2040
Conversation
visitTypeParameter(generic); | ||
} | ||
} catch (Throwable ignore) { | ||
// partial classpath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we catch more exact exception? Such error handling may cause big problems during future debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do this
visitTypeReference(CtRole.INTERFACE, anInterface); | ||
try { | ||
for (Type anInterface : clazz.getGenericInterfaces()) { | ||
visitTypeReference(CtRole.INTERFACE, anInterface); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to try/catch this line only. So some interfaces are correctly created and some are ignored. Actually it process all interfaces until first which is not on classpath.
Do the same in all cycles of this method, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot the Error is thrown by clazz.getGenericInterfaces()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I did not succeed to find a way to get a partial list :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So then try/catch only clazz.getGenericSuperclass(). So the exceptions thrown in visitTypeReference are not catched
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the motivation to do it, right now.
There is probably 50 invocations to protect in this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be written in nice way using lambda:
for (Type anInterface : ignoreMissingClass(()->clazz.getGenericInterfaces()))
where ignoreMissingClass will return empty collection in case of exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clazz.getGenericInterfaces() returns an array not a collection.
I don't see a way to do it nicely :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<T> T[] ignoreMissingClass(Supplier<T[]> supl) {
try {
return supl.get();
} catch (Throwable e) {
//TODO check if it is expected problem, else rethrow
return EMPTY_ARR;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the problem is to create EMPTY_ARR, because EMPTY_ARR has to be different for each T
try { | ||
for (Annotation annotation : clazz.getDeclaredAnnotations()) { | ||
visitAnnotation(annotation); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This for cycle seems to be copy past here by mistake. It is already above. Please fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand it is the only loop on clazz.getDeclaredAnnotations() for visitEnum(Class clazz).
Did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See line 142-144 in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not the same method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. First one processes Interface and second one processes Enum. You are right. It is correct. Thanks for explanation.
LGTM. |
It is a little bit dirty, do you have better idea how to do it?
Fix #2038