refactor: Remove method get_global_jclass #580
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viirya
reviewed
Jun 20, 2024
viirya
reviewed
Jun 20, 2024
viirya
reviewed
Jun 20, 2024
viirya
reviewed
Jun 20, 2024
The method uses unsafe code to cast a value to a static lifetime using std::mem::transmute. But none of users of the method actually seems to depend on this lifetime extention. So to me it seems better to just call the underlying `find_class` method and delete the unsafe code.
Remove no longer needed comments. Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
eejbyfeldt
force-pushed
the
remove-get-global-jclass
branch
from
e61d948
to
8d3c0c5
Compare
viirya
approved these changes
Jun 21, 2024
|
Merged. Thanks @eejbyfeldt @andygrove @sunchao |
himadripal
pushed a commit
to himadripal/datafusion-comet
that referenced
this pull request
Sep 7, 2024
* refactor: Remove method get_global_jclass The method uses unsafe code to cast a value to a static lifetime using std::mem::transmute. But none of users of the method actually seems to depend on this lifetime extention. So to me it seems better to just call the underlying `find_class` method and delete the unsafe code. * Apply suggestions from code review Remove no longer needed comments. Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> --------- Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The method uses unsafe code to cast a value to a static lifetime using
std::mem::transmute. But none of users of the method actually seems to depend on this lifetime extention. So to me it seems better to just call the underlyingfind_classmethod and delete the unsafe code.Which issue does this PR close?
We needed to change this code in #570 when upgrading to rust 1.79 and that highlights that usage of transmute is fishy.
Rationale for this change
Less code and less unsafe which makes it easier to check and understand that the code is correct.
What changes are included in this PR?
Removed the method
get_global_jclass. All users could be changed to use the underlying methodfind_classwithout further changes.How are these changes tested?
Existing tests.