New warnings from -Wconversion -Wno-sign-conversion

[eddelbuettel]

Prof Ripley emailed the following in a messaging concerning the earlier R_NO_MAP issue (that is apparently defered for a bit at CRAN, we did our bit here, he pointed me also to another issue in another package of mine now taken care of):

A couple of other warnings I noted whilst compiler logs for CRAN packages scrolled by:

/Users/ripley/R/Library/Rcpp/include/Rcpp/sugar/functions/sample.h:369:17: 
warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 
'long') to 'int' [-Wshorten-64-to-32]

/Users/ripley/R/Library/Rcpp/include/Rcpp/internal/export.h:114:9: 
warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 
'long') to 'uword' (aka 'unsigned int') [-Wshorten-64-to-32]

(That's from clang with -Wconversion -Wno-sign-conversion.)

[eddelbuettel]

I am actually not sure I can reproduce these with clang++-17, at least not from building Rcpp. uword hints at RcppArmadillo but it could also be any one of 1100+ packages using it. Uggh.

[Enchufa2]

I assume you asked Prof. Ripley for more details. :) In the meantime, I found this for the second one via Google search, nothing for the first one.

[eddelbuettel]

I looked into this some more. One can tickle a fair amount by for example testing our embedded package testRcppClass via the test file test_module_client_package.R. And while I can subsitute a number of int for size_t invariably I end at an impassed that I convert enough to silence all warnings, the module no longer loads. Uggh. One can backtrack and only convert 'some' and the module remains loadable, but so does a faily warning. The main culprit (for this issue) appears to be this function. I changed some more trivial things (adding void to signatures in init.c for the test package, a number of other int -> size_t in class.h) but this one stings.

So we -Wconversion -Wno-sign-conversion under clang are enforced we may have some trouble.

[Enchufa2]

I looked into this some more. One can tickle a fair amount by for example testing our embedded package testRcppClass via the test file test_module_client_package.R. And while I can subsitute a number of int for size_t invariably I end at an impassed that I convert enough to silence all warnings, the module no longer loads.

Do you have a branch with this to apply another pair of eyes?

[eddelbuettel]

~Hah! Following #1303 this is now different and (for the one test I looked at) much quieter. Will keep looking.~ Not so. I forgot the required setters for expanded tests.

And lo and behold it is in better shape now as #1303 helped us here too it seems. To recap, I used this is ~/.R/Makevars` to get the appropriate compiler and the two important flags:

CLANGVER=-17
#CLANGLIB=-stdlib=libc++
CXX=$(CCACHE) clang++$(CLANGVER) $(CLANGLIB)
CXX11=$(CCACHE) clang++$(CLANGVER) $(CLANGLIB)
CXX14=$(CCACHE) clang++$(CLANGVER) $(CLANGLIB)
CXX17=$(CCACHE) clang++$(CLANGVER) $(CLANGLIB)
CXX20=$(CCACHE) clang++$(CLANGVER) $(CLANGLIB)
CC=$(CCACHE) clang$(CLANGVER)
SHLIB_CXXLD=clang++$(CLANGVER) $(CLANGLIB)
CXXFLAGS=-Wall -O3 -pedantic -Wconversion -Wno-sign-conversion #-Wno-unused-but-set-variable -Wno-delee-non-abstract-non-virtual-dtor
CXX11FLAGS=-Wall -O3 -pedantic -Wconversion -Wno-sign-conversion #-Wno-unused-but-set-variable -Wno-delete-non-abstract-non-virtual-dtor
CXX14FLAGS=-Wall -O3 -pedantic -Wconversion -Wno-sign-conversion #-Wno-unused-but-set-variable -Wno-delete-non-abstract-non-virtual-dtor
CXX17FLAGS=-Wall -O3 -pedantic -Wconversion -Wno-sign-conversion #-Wno-unused-but-set-variable -Wno-delete-non-abstract-non-virtual-dtor
CXX20FLAGS=-Wall -O3 -pedantic -Wconversion -Wno-sign-conversion #-Wno-unused-but-set-variable -Wno-delete-non-abstract-non-virtual-dtor

With that I ran the following snippet repeatedly (as we by the design of our tests have to reinstall to have the updated headers used) from directory inst/tinytest.r

cd ../..; ./cleanup; install.r; cd -; RunVerboseRcppTests=yes RunAllRcppTests=yes tt.r -f test_module_client_package.R

where install.r and tt.r are simple helpers from littler to run R CMD INSTALL with preferred flags and to run a single test file via tinytest (i.e. tinytest::run_test_file(filename)).

[eddelbuettel]

New branch bugfix/size_t_casts pushed.

It behaves here. I think the key difference to what gave me minor trouble yesterday is the one-line change in Module.h.

[Enchufa2]

(Note to self) Packages to check for more casts: support, lavacreg, hdtg, mapdeck, morf

Found using this search:

site:www.stats.ox.ac.uk /Users/ripley/R/Library/Rcpp/include/Rcpp warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]

[JanMarvin]

Hi, not sure if further input is still needed, but I saw the following warning when compiling with clang 17.0.6 and -Wconversion -Wno-sign-conversion

/home/jmg/R/x86_64-pc-linux-gnu-library/4.4/Rcpp/include/Rcpp/internal/Proxy_Iterator.h:110:23: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'difference_type' (aka 'int') [-Wshorten-64-to-32]
  110 |                         return proxy.index - other.proxy.index ;
      |                         ~~~~~~ ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

This I was able to silence switching int to R_xlen_t here: https://github.com/RcppCore/Rcpp/blob/b6941183db941c0d501b0af0502170b2b45b075b/inst/include/Rcpp/internal/Proxy_Iterator.h#L34

[eddelbuettel]

Hi @JanMarvin that is helpful as we likely do need additional rounds of fixes as different packages will expose different parts of the "API surface". Which package did you compile?

[JanMarvin]

Hi @eddelbuettel , I was compiling openxlsx2. The warning is triggered by a custom wrap() function in a _types.h file with #include <RcppCommon.h>

[eddelbuettel]

Confirming. An alternative (more local?) fix would be

@@ -107,7 +105,7 @@ public:
 		}
 
 		inline difference_type operator-(const Proxy_Iterator& other) const {
-			return proxy.index - other.proxy.index ;
+			return static_cast<difference_type>(proxy.index - other.proxy.index) ;
 		}
 
 		inline int index() const { return proxy.index ; }

which is more local but your suggestion is likely better. But may require more testing.

[eddelbuettel]

@Enchufa2 You search string is good! I see

• [x] support: no longer on CRAN
• [x] VBel: lots of warnings from RcppEigen (another time ...), the one reported on Vector does not reproduce anymore so yay us!
• [x] lavacreg: false positive in search, package compilation log no longer there
• [x] hdtg: only from other packages: RcppEigen, RcppXsimd, RcppParallel, own package source code
• [x] mapdeck: false positive in search, package compilation log no longer there
• [x] morf: no longer on CRAN

I will try to get to these one by one. Help welcome :grinning: . I will also push a new branch with the change suggested by @Janmarvin and we can combine changes in there.

PS We could fetch support and morf from their Archive sections ...

[eddelbuettel]

That worked out ok in the rev.dep check so I will make another PR.

[eddelbuettel]

Closing as #1309 covered this.