HenrikBengtsson commented 12 months ago

From: Kurt Hornik Date: Sun, Nov 26, 2023 at 11:59 PM Subject: CRAN package matrixStats To: henrikb@braju.com Cc: CRAN Team

Dear maintainer,

Please see the problems shown on https://cran.r-project.org/web/checks/check_results_matrixStats.html.

Please correct before 2023-12-11 to safely retain your package on CRAN.

Best, -k

Details

  Found the following significant warnings:
    binCounts.c:25:81: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    binMeans.c:26:60: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    binMeans.c:26:66: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    binMeans.c:33:81: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    colOrderStats.c:58:47: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    indexByRow.c:30:68: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    indexByRow.c:32:62: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    indexByRow.c:32:66: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    indexByRow.c:69:93: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘double’ [-Wformat=]
    rowOrderStats.c:58:47: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    weightedMean.c:26:60: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    weightedMean.c:26:66: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    weightedMedian.c:26:60: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
    weightedMedian.c:26:66: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=]
  See ‘/home/hornik/tmp/R.check/r-devel-gcc/Work/PKGS/matrixStats.Rcheck/00install.out’ for details.
  * used C compiler: ‘gcc-13 (Debian 13.2.0-5) 13.2.0’

HenrikBengtsson commented 12 months ago

Quick comment: The solution should be to use %ld instead of %d.

HenrikBengtsson commented 12 months ago

I can reproduce this locally using R CMD check --as-cran with R-devel (2023-11-27 r85642) and gcc (Ubuntu 11.4.0-1ubuntu1\~22.04) 11.4.0.

MS Windows R-devel (2023-11-26 r85638 ucrt) with gcc.exe (GCC) 12.3.0 detects it. Linux R-devel (2023-11-22 r85609) and gcc (Ubuntu 11.4.0-1ubuntu1\~22.04) 11.4.0 on GitHub Actions does not detect it.

HenrikBengtsson commented 12 months ago

Fixed it for Linux R-devel, but on MS Windows, I get:

* using R Under development (unstable) (2023-11-26 r85638 ucrt)
* using platform: x86_64-w64-mingw32
* R was compiled by
    gcc.exe (GCC) 12.3.0
    GNU Fortran (GCC) 12.3.0
* running under: Windows Server 2022 x64 (build 20348)
...
* checking whether package 'matrixStats' can be installed ... [25s] WARNING
Found the following significant warnings:
  binCounts.c:25:82: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  binMeans.c:26:61: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  binMeans.c:26:68: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  binMeans.c:33:82: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  colOrderStats.c:58:48: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  indexByRow.c:30:69: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  indexByRow.c:32:63: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  indexByRow.c:32:68: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  rowOrderStats.c:58:48: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  weightedMean.c:26:61: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  weightedMean.c:26:68: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  weightedMedian.c:26:61: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
  weightedMedian.c:26:68: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'R_xlen_t' {aka 'long long int'} [-Wformat=]
See 'D:/a/matrixStats/matrixStats/check/matrixStats.Rcheck/00install.out' for details.
* used C compiler: 'gcc.exe (GCC) 12.2.0'

which is also what's reported on CRAN.

HenrikBengtsson commented 12 months ago

So, R_xlen_t is defined as:

typedef ptrdiff_t R_xlen_t;

and ptrdiff_t depends on platform and compiler, which explains why we have R_xlen_t being long int on Linux, but long long int on MS Windows.

There is printf %td flag specifically designed for the 'ptrdiff_t' type.

That works on Linux, but not on MS Windows (running R-devel (2023-11-26 r85638 ucrt with gcc.exe (GCC) 12.3.0), where we get warnings like:

 binMeans.c:26:60: warning: unknown conversion type character 't' in format [-Wformat=]

So, using %td is unfortunately not an option. It looks like we need to come up with our own agility where we use either %ld or %lld depending on sizeof(R_xlen_t).

HenrikBengtsson commented 12 months ago

Looking at the R-devel commit history, I found that:

# define R_PRIdXLEN_T "td"

was introduced three hours ago, cf. https://github.com/wch/r-source/commit/38403c9c347dd5426da6009573b087188ec6be04.

Should probably wait a few days to see how this develops.

HenrikBengtsson commented 12 months ago

Using %lld with explicit (long long int) coersions seems to work, cf. https://github.com/HenrikBengtsson/matrixStats/actions/runs/7010218329/job/19070259510.

yaccos commented 12 months ago

I am a bit curious of why the CRAN checks has not complained of this in the past as the code which had to be changed has been in the package for years without any issue. Is there a change in the R devel version which is triggering the warning?

HenrikBengtsson commented 12 months ago

Is there a change in the R devel version which is triggering the warning?

Yes, this was introduced to R-devel recently. This is typically how R Core/CRAN is working - they add more and more checks over time. So, it's only now these mistakes are caught. In this case, I think they're inspecting gcc output.

HenrikBengtsson commented 11 months ago

Summary

%td will work for R (>= 4.2.0)
%lld + (long long int) or %d + (double) should work everywhere.

Because matrixStats supports older versions of R too, specifically R (>= 2.12.0), we cannot use %td. The easiest is to use %lld, which is now what is used in the develop branch.

Details

Excerpts from the R-package-devel thread '[R-pkg-devel] Canonical way to Rprintf R_xlen_t' started (https://stat.ethz.ch/pipermail/r-package-devel/2023q4/010123.html) on 2023-11-28:

Tomas Kalibera wrote: "Please let me clarify. %td works in R on Windows in R 4.3 and R-devel, when using the recommended toolchain, which is Rtools43. It also worked with R 4.2 and Rtools42. It works since R has switched to UCRT on Windows." [...] "There is a bug in GCC, still present in gcc 12 and gcc 10, due to which gcc displays warnings about the format even when it is supported. [...] I've been patching GCC in Rtools42 and Rtools43 to avoid this problem, so you don't get the warning there. My patch has been picked up also by Msys2 ...". "An unpatched GCC 10 or 12 with UCRT will print a warning for %td but will support it."
Tomas Kalibera wrote: %td would not work with R < 4.2 built using the then recommended toolchains, because MSVCRT was used, which did not support %td. The runtime library would not be able to print such value." and "For such old versions of R, you would have to do something else. I think PRId64 from inttypes.h and a cast to "(long long)" should work. The macro would expand to the Microsoft format pattern in the older versions. Or depending on what you want to do, also you could simply cast to double and print that way."

HenrikBengtsson / matrixStats

CRAN: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘R_xlen_t’ {aka ‘long int’} [-Wformat=] (due 2023-12-11) #244

Details

Summary

Details