Remove cast to R_xlen_t (closes #1334) by eddelbuettel · Pull Request #1335 · RcppCore/Rcpp (original) (raw)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A related question is whether the two const interators on lines 336 and 338 should also get the cast.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels certainly wrong that there is a cast only there. What image did you use to reproduce the warnings?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, given that this was address in two steps (one, two), and this line was part of the first PR, is it possible that, after the second set of changes, the static_cast is not needed anymore? Because size() already returns R_xlen_t. (Specifically, after this change).
We should be fine just dropping the cast.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so too. We shuld be able to drop the cast.
I just installed clang++-17, at the time likely on Debian maybe using unstable. It is currently available in my default Ubuntu 24.04 setup.
There are other packages having issues with clang++-18 and now clang++-19 so we should maybe try those.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues with clang++-19 with the compiler flags that tickled this, either in installation or check. Here is installation (in a rocker/r-base container using Debian testing/unstable)
root@e47796620ac8:/work# install.r
- installing source package found in current working directory ...
- installing source package ‘Rcpp’ ... ** using staged installation ** libs using C++ compiler: ‘Debian clang version 19.1.0 (4)’ clang++-19 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -fpic -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion -c api.cpp -o api.o clang++-19 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -fpic -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion -c attributes.cpp -o attributes.o clang++-19 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -fpic -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion -c barrier.cpp -o barrier.o clang++-19 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -fpic -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion -c date.cpp -o date.o clang++-19 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -fpic -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion -c module.cpp -o module.o clang++-19 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -fpic -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion -c rcpp_init.cpp -o rcpp_init.o clang++-19 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-z,relro -o Rcpp.so api.o attributes.o barrier.o date.o module.o rcpp_init.o -L/usr/lib/R/lib -lR installing to /usr/local/lib/R/site-library/00LOCK-work/00new/Rcpp/libs ** R ** inst ** byte-compile and prepare package for lazy loading ** help *** installing help indices ** building package indices ** installing vignettes ** testing if installed package can be loaded from temporary location ** checking absolute paths in shared objects and dynamic libraries ** testing if installed package can be loaded from final location ** testing if installed package keeps a record of temporary installation path
- DONE (Rcpp) root@e47796620ac8:/work#
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(To clarify, that was after I removed the cast.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now pushed it. I will take the slow road here and let the current (2 1/2 day total !!) rev.dep test finish, fill-in missing build dependencies it uncovers and then re-run again with the cast removed.