Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary boxing #7539

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Remove unnecessary boxing #7539

merged 2 commits into from
Dec 13, 2024

Conversation

KangarooKoala
Copy link
Contributor

Also remove unnecessary @SuppressWarnings("overloads") in NumericalIntegration.java.

I'm not sure if the unboxing will mess with the JUnit4 Parameterized tests, since it appears to use reflection so compiling does not necessarily mean there will not be an error when running. (I can't run ./gradlew wpilibjIntegrationTests:run locally since it couldn't wpiHaljni in java.library.path) Given the more or less abandoned status of wpilibjIntegrationTests, I suspect it will be fine to merge this in and on the off chance we actually do use wpilibjIntegrationTests again and this PR does mess with it, we'll just revert the change then.

@KangarooKoala
Remove unnecessary boxing
5038c1a 
Also remove unnecessary warning suppression
@KangarooKoala KangarooKoala requested review from PeterJohnson and a team as code owners December 12, 2024 01:28
@github-actions github-actions bot added component: wpiutil WPI utility library component: wpilibj WPILib Java component: wpimath Math library labels Dec 12, 2024
calcmogul
calcmogul previously approved these changes Dec 12, 2024
View reviewed changes
SamCarlberg
SamCarlberg reviewed Dec 12, 2024
View reviewed changes
Copy link
Member

@SamCarlberg SamCarlberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few methods that return a Long, too, but I'm not at a computer right now to look them up

wpimath/src/main/java/edu/wpi/first/math/system/NumericalIntegration.java Outdated Show resolved Hide resolved
@KangarooKoala
Copy link
Contributor Author

I think there are a few methods that return a Long

Correct. A grep for '[ (]Long ' in allwpilib's Java files brought up CameraServerShared.getRobotMainThreadId() and the implementations in CameraServerSharedStore and RobotBase. However, I can't change the return type to a long because the default implementation (in CameraServerSharedStore) returns null, which can't be expressed with a primitive long. Maybe it'd be fine to change it to an OptionalLong, though? It'd be a breaking change, but it also shouldn't affect many people. (Probably just us, actually)

I'm also not sure how the MRC in 2027 affects the relative benefits and costs of replacing the Long with an OptionalLong, since I don't know how much of the camera server stuff will stay and how much will be nuked. I'm guessing this part would stay, but I'm not sure.

@SamCarlberg
Copy link
Member

I doubt anyone uses those cameraserver APIs; we can leave it alone

@PeterJohnson PeterJohnson merged commit 4225b73 into wpilibsuite:main Dec 13, 2024
43 checks passed
@KangarooKoala KangarooKoala deleted the unnecessary-boxing branch December 13, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calcmogul calcmogul calcmogul left review comments

@SamCarlberg SamCarlberg SamCarlberg approved these changes

@PeterJohnson PeterJohnson Awaiting requested review from PeterJohnson PeterJohnson is a code owner

Assignees
No one assigned
Labels
component: wpilibj WPILib Java component: wpimath Math library component: wpiutil WPI utility library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KangarooKoala @SamCarlberg @calcmogul @PeterJohnson