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

feat: add float32 and float64 support #14

Merged
merged 3 commits into from
Sep 7, 2024
Merged

feat: add float32 and float64 support #14

merged 3 commits into from
Sep 7, 2024

Conversation

ccoVeille
Copy link
Owner

@ccoVeille ccoVeille commented Sep 6, 2024

Fixes #7

@ccoVeille ccoVeille requested a review from ldemailly September 6, 2024 21:34
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f7927b7) to head (328cfff).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #14   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           54        60    +6     
=========================================
+ Hits            54        60    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ccoVeille ccoVeille changed the title feat: add float32 and float63 support feat: add float32 and float64 support Sep 6, 2024
conversion.go Outdated Show resolved Hide resolved
conversion.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@ccoVeille ccoVeille merged commit 3049cd5 into main Sep 7, 2024
5 checks passed
@ccoVeille ccoVeille deleted the float branch September 7, 2024 22:32
@ldemailly
Copy link
Collaborator

it's a bit odd to merge something that only covers 1 case (float to uint) and not all the others? hopefully you don't tag this

@ccoVeille
Copy link
Owner Author

it's a bit odd to merge something that only covers 1 case (float to uint) and not all the others? hopefully you don't tag this

This PR allowed float32/64 to all integer types.

Were you talking about having a way to convert any int to float32/64. There is no overflow for these ?

As far as I know the only other overflow I'm not supporting is float64 to float32.

I might add them later.

Everything can be stored into a float64, so no overflow.

@ldemailly
Copy link
Collaborator

where is the check for overflow for say uint32

and none of the signed overflow are checked

@ccoVeille
Copy link
Owner Author

ccoVeille commented Sep 8, 2024

where is the check for overflow for say uint32

Are you talking about a bug in the code or a missing test?

Are you talking about the test I made when converting a float32/flpat64 to uint32?

Could you spot the exact line in the PR where I should have added something?

Maybe you are talking about the fact the code will cast math.MaxFloat32 with uint64 and compare it to math.MaxUint32

This might be the issue you were talking because yes, the code and tests might have a bug.

But I think refactoring in might address it #18, I may have to split the PR indeed

none of the signed overflow are checked

I noticed and I addressed it (I think) in #18

@ccoVeille
Copy link
Owner Author

ccoVeille commented Sep 8, 2024

Maybe you are talking about the fact the code will cast math.MaxFloat32 with uint64 and compare it to math.MaxUint32\n\nThis might be the issue you were talking because yes, the code and tests might have a bug.

Comfirmed https://go.dev/play/p/3DlJcb10eYs

But it's fixed via #18, I might reorganize the commits of this PR to fix the bug before refactoring to add the new error message

@ccoVeille
Copy link
Owner Author

Follow up will be in #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handle the float conversion to int
3 participants