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

add cast to xlength #5929

Merged
merged 2 commits into from
Feb 22, 2024
Merged

add cast to xlength #5929

merged 2 commits into from
Feb 22, 2024

Conversation

ben-schwen
Copy link
Member

@ben-schwen ben-schwen commented Jan 31, 2024

Follow up to #5768
Closes #5935
Fixes additional M1mac issue in CRAN check results

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6f43a96) 97.48% compared to head (e9f6cf8) 97.48%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5929   +/-   ##
=======================================
  Coverage   97.48%   97.48%           
=======================================
  Files          80       80           
  Lines       14862    14862           
=======================================
  Hits        14488    14488           
  Misses        374      374           

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

@MichaelChirico
Copy link
Member

Shall we target a new 1-15-2 patch branch for this? @TysonStanley have they e-mailed you about needing to fix this quickly?

@MichaelChirico
Copy link
Member

Would adding an arm64 runner have helped here? Newly supported in GHA:

https://fosstodon.org/@gaborcsardi/111850319359214546

@TysonStanley
Copy link
Member

Shall we target a new 1-15-2 patch branch for this? @TysonStanley have they e-mailed you about needing to fix this quickly?

They haven't. And the checks all are saying ok even tho that issue is reported below so not sure what their protocol is for that. It popped up in the 1.14.10 release right at the same time we were submitting 1.15.

@jangorecki
Copy link
Member

As long as CRAN doesn't ask for fix I think we can keep it in 1.15.99. Over time there may be some extra stuff needed for patch release and then we can push that together.

@ben-schwen
Copy link
Member Author

ben-schwen commented Feb 1, 2024

AFAIA CRAN is trying out new checks for some time which are not hard checks right now but might become in the future. At least they are now fair enough to provide details on the setup of the checks (before it was some obscure E-mail telling you that your programming is wrong :D)

Keeping it in 1.15.99 should be fine.

@tdhock
Copy link
Member

tdhock commented Feb 6, 2024

there was a related message from Ivan Krylov on R-devel today, quoted below.

Apologies for not answering the question you asked, but is this about
hdf5r and problems printing R_xlen_t [*] that appeared in 1.3.8 and you
tried to solve in 1.3.9?

We had a thread about this last November:
https://stat.ethz.ch/pipermail/r-package-devel/2023q4/010123.html

To summarise, there is no single standard C format specifier that can be
used to print R_xlen_t. As an implementation detail, it can be defined
as int or ptrdiff_t (or something completely different in the future),
and ptrdiff_t itself is usually defined as long or long long (or, also,
something completely different on a weirder platform). All three basic
types can have different widths and cause painful stack-related
problems when a mismatch happens.

In R-4.4, there will be a macro R_PRIdXLEN_T defining a compatible
printf specifier. Until then (and for compatibility with R-4.3 and
lower), it's relatively safe to cast to (long long) or (ptrdiff_t) and
then use the corresponding specifier, but that's not 100% future-proof.
Also, mind the warnings that mingw compilers sometimes emit for "new"
printf specifiers despite UCRT is documented to support them.

@ben-schwen ben-schwen marked this pull request as ready for review February 19, 2024 09:42
@MichaelChirico MichaelChirico merged commit 74db860 into master Feb 22, 2024
2 checks passed
@MichaelChirico MichaelChirico deleted the wformat_m1 branch February 22, 2024 05:55
@MichaelChirico
Copy link
Member

Also cherry-picked to the 1-15-2 branch, where I also added a NEWS item:

52220da

e4912e9

@jangorecki jangorecki added this to the 1.15.2 milestone Feb 22, 2024
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.

data.table 1.15.0 two failing tests on CRAN
5 participants