search

RcppCore / Rcpp

Seamless R and C++ Integration
https://www.rcpp.org
GNU General Public License v2.0
723 stars 208 forks source link

New warnings from -Wconversion -Wno-sign-conversion #1299

Open eddelbuettel opened 3 months ago

eddelbuettel commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

~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 commented 1 month ago

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 commented 1 month ago

(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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@Enchufa2 You search string is good! I see

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 commented 4 weeks ago

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