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

fix bit64 coercion in memrecycle #5835

Merged
merged 4 commits into from
Dec 19, 2023
Merged

fix bit64 coercion in memrecycle #5835

merged 4 commits into from
Dec 19, 2023

Conversation

ben-schwen
Copy link
Member

@ben-schwen ben-schwen commented Dec 18, 2023

Closes #5834
Follow up to #5189

Type coercion is from double[numeric] into double[integer64].
Previously we checked int(val)!=val for detecting truncation.
Since integer64 can handle much longer values the correct check is (int64_t)val!=val.

Alters two tests since no truncation/precision loss takes place.

@jangorecki does this need a NEWS item?

@jangorecki jangorecki added this to the 1.16.0 milestone Dec 18, 2023
@jangorecki
Copy link
Member

No need NEWS info. It's was just a warning, behavior is not really changed.

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (34e02f2) 97.46% compared to head (bd0beea) 97.46%.

❗ Current head bd0beea differs from pull request most recent head f90bca0. Consider uploading reports for the commit f90bca0 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5835   +/-   ##
=======================================
  Coverage   97.46%   97.46%           
=======================================
  Files          80       80           
  Lines       14822    14822           
=======================================
  Hits        14447    14447           
  Misses        375      375           

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

@jangorecki
Copy link
Member

any idea if bit64 integer64 type can handle 64bit or 53bit of data? If it is based on R's double then probably only 53bit. Then testing int64_t(val)!=val may not be really accurate

@ben-schwen
Copy link
Member Author

any idea if bit64 integer64 type can handle 64bit or 53bit of data? If it is based on R's double then probably only 53bit. Then testing int64_t(val)!=val may not be really accurate

According to documentation and implementation they can handle [-2^63+2, 2^63-1] which makes sense, since -2^63+1 (LLONG_MIN) represents NA_INTEGER64

@jangorecki jangorecki modified the milestones: 1.16.0, 1.15.0 Dec 18, 2023
@tdhock
Copy link
Member

tdhock commented Dec 18, 2023

hi @ben-schwen thanks for the PR. Please consider volunteering as a reviewer for assign.c (add a line to CODEOWNERS, nobody has volunteered for that file yet).

@ben-schwen ben-schwen marked this pull request as ready for review December 18, 2023 22:13
@HughParsonage
Copy link
Member

I propose that this PR be now merged so that my PR (which will conflict) can be finalized.

@MichaelChirico
Copy link
Member

nit about dropping c() for length-1 literal, LGTM otherwise.

jangorecki and others added 2 commits December 19, 2023 07:23
Co-authored-by: Michael Chirico <chiricom@google.com>
Co-authored-by: Michael Chirico <chiricom@google.com>
@jangorecki jangorecki merged commit 6c1fd83 into master Dec 19, 2023
2 checks passed
@ben-schwen ben-schwen deleted the coerceAs_bit64 branch July 8, 2024 20:19
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.

coerceAs to int64 unexpected warning
5 participants