New warnings from -Wconversion -Wno-sign-conversion
Opened this issue ยท 13 comments
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.)
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.
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.
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.
I looked into this some more. One can tickle a fair amount by for example testing our embedded package
testRcppClass
via the test filetest_module_client_package.R
. And while I can subsitute a number ofint
forsize_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?
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)
).
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.
(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]
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:
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?
Hi @eddelbuettel , I was compiling openxlsx2
. The warning is triggered by a custom wrap()
function in a _types.h
file with #include <RcppCommon.h>
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.
@Enchufa2 You search string is good! I see
- support: no longer on CRAN
- VBel: lots of warnings from RcppEigen (another time ...), the one reported on Vector does not reproduce anymore so yay us!
- lavacreg: false positive in search, package compilation log no longer there
- hdtg: only from other packages: RcppEigen, RcppXsimd, RcppParallel, own package source code
- mapdeck: false positive in search, package compilation log no longer there
- morf: no longer on CRAN
I will try to get to these one by one. Help welcome ๐ . 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 ...
That worked out ok in the rev.dep check so I will make another PR.