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.

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

Cleanup the use of Float primitive wrapper. #3890

Merged

Conversation

BradWalker
Copy link
Member

@BradWalker BradWalker commented Mar 30, 2022

As of Java 9, the VM can do a better job of handling the conversion from a float primitive to a Float Object or vice versa.

This is an extension of the work done in #2498.

No tests have been touched as those expressly need to manage the conversion.

Lastly, these changes are project wide and not module specific.


^Add meaningful description above

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

…do a better job of handling the

coversion from a float primitive to a Float Object or vice versa.

This is an extension of the work done in apache#2498.

No tests have been touched as those expressly need to manage the conversion.
@BradWalker BradWalker self-assigned this Mar 30, 2022
@BradWalker BradWalker added this to the NB14 milestone Mar 30, 2022
@BradWalker BradWalker added Code cleanup Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) labels Mar 30, 2022
@BradWalker BradWalker requested a review from lkishalmi March 30, 2022 00:33
@mbien
Copy link
Member

mbien commented Mar 31, 2022

how do you select the sub-projects you are cleaning up? I am wondering if it would be better to continue adding more commits to this PR until it is complete and all types are covered in the files you touched. So that we can review the switch blocks once instead of 8 times :)

@BradWalker
Copy link
Member Author

BradWalker commented Mar 31, 2022

how do you select the sub-projects you are cleaning up? I am wondering if it would be better to continue adding more commits to this PR until it is complete and all types are covered in the files you touched. So that we can review the switch blocks once instead of 8 times :)

Hey @mbien ,

Most of the "rust removal" work that I do, is done to try and make it easy for a reviewer. For example, the "new Float" work this change covers should just be about Floats. I find it easier for me to make changes this way and if I had to review, it makes things easier for me to think in those terms.

Changes required for the "new Double" are going to be really large (i.e. lots of different files). So that really should stand on it's own.

Also, I did try and bundle together the Byte & Short changes since they were reasonable to do and review.

Can I count you as an approver? 8-)

BTW, hang in there.. I'm almost done..

@BradWalker
Copy link
Member Author

Hey @lkishalmi , can I count you as a reviewer of this?

Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

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

Well, this is fine for me. Also I'd rather see these PR-s as separated work even if I have to review the same case 8 times.

My only addition that it would be good to pick and use one notation instead of: 0f, 0F, 0.0F
(where, I might pick: 0.0F, as I'm old enough that my brain pick 0f or 0F as a hexadecimal number.)

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

Can I count you as an approver? 8-)

yes, changes look good.

Is this project-wide for all new Float() occurrences or have you selected specific clusters to clean up? If its not project-wide, it would be good mentioning it in the commit which areas it covers.

@BradWalker BradWalker merged commit 190ffcb into apache:master Apr 3, 2022
@BradWalker BradWalker deleted the cleanup_float_primitive_wrapper branch April 3, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code cleanup Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants