-
Notifications
You must be signed in to change notification settings - Fork 337
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
Modify Types.instanceOf to take a ClassInfo as LHS #410
Modify Types.instanceOf to take a ClassInfo as LHS #410
Conversation
This method's declaration currently reads `instanceOf(String, String)`, taking both the LHS and RHS types as type signature strings. As a result, to use a `ClassInfo` object `ci` as the LHS, a caller must convert it into a string via `ci.getType()` (see `ElementInfo.instanceOf`), before the string eventually gets turned back into a `ClassInfo` object (if it's a reference type) in the method body. This patch changes the method declaration to read `instanceOf(ClassInfo, String)`. This change not only saves the back-and-forth, but also addresses the case where the LHS class has been loaded by a non-default class loader, in which case the original implementation fails to resolve its type string back into a `ClassInfo`. One downside is that if LHS is a nested array, the new implementation will resolve a `ClassInfo` object for every "layer" of the array (via `ClassInfo.getComponentClassInfo`), while the original implementation "peels" the array via (cheaper) calls to `String.substring`. I'm guessing this performance degradation will not be noticeable in common scenarios. This patch also simplifies the implementation of `instanceOf`, ensures it recognizes arrays as subtypes of Cloneable and Serializable (JLS 4.10.3), changes the names of a few variables to clarify they're expected to contain type signatures, and adds unit tests (to `ClassTest.java`, where the existing `testInstanceOf` unit test resides).
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.
Thanks, I think that if it's possible to avoid ASM, then it is better to omit a test. Perhaps we can later have an equivalent test without ASM.
@pparizek , WDYT?
Maybe the relevant classes (example.MyClass1, example.MyClass2) can be added in the form of real class files as resources for tests, instead of generating them at runtime with ASM. |
Yes, I think that would be best. I'll create an issue for that.
…On Wed, Aug 16, 2023 at 2:02 PM Pavel Parizek ***@***.***> wrote:
Maybe the relevant classes (example.MyClass1, example.MyClass2) can be
added in the form of real class files as resources for tests, instead of
generating them at runtime with ASM.
—
Reply to this email directly, view it on GitHub
<#410 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABXV4R7GEVDR4S7WK3ELPGTXVSZGRANCNFSM6AAAAAA3QGOZOI>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Regards,
Cyrille Artho
|
@zhangwen0411 , can you please add two empty classes with suitable package names to |
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.
If possible, let's add the two classes the normal way, as I've described in issue #411.
I see. In that case, I think it's best to add the extra test as is now, and we can see later if the test can be rewritten later. |
Subsumed by PR #415 (which we wanted to merge now as some other patches depend on it). Thank you for providing this PR, which served us as a template. |
This PR is a port of #388 to the
java-10-gradle
branch.I omitted a test case (f575504) that depends on the ASM framework, a dependency added to the
master
branch in #203 but not present in thejava-10-gradle
branch. I'd be happy to add the dependency and the test case if that's preferred!