Repository navigation
Consider -Wconversion warnings #1488
Description
Enabling -Wconversion (particularly with clang++ as on the M1mac machine at CRAN, see its README) exposes a number of warnings. The Oxford machine uses clang++-21, on Ubuntu 26.04 we also have clang++-22 which Vienna uses (without the same options). Per the README this uses
CFLAGS="-falign-functions=8 -g -O2 -Wall -pedantic -Wconversion -Wno-sign-conversion -Wstrict-prototypes"
C17FLAGS="-falign-functions=8 -g -O2 -Wall -pedantic -Wconversion -Wno-sign-conversion -Wno-strict-prototypes"
# ...
CXXFLAGS="-g -O2 -Wall -pedantic -Wconversion -Wno-sign-conversion"
Note that this turns 'sign-conversion' warnings off. Those account for a ton. I found first set of changes we could make by not doing this i.e. by running with only -Wconversion. That may be too radical, but it could still hide some overflows.
Also, our own compilation is 'clean' under -Wconversion -Wno-sign-conversion however other package may see
In file included from /Users/ripley/R/Library/Rcpp/include/Rcpp/sugar/functions/functions.h:63:
/Users/ripley/R/Library/Rcpp/include/Rcpp/sugar/functions/mean.h:38:14: warning: implicit conversion from 'R_xlen_t' (aka 'long') to 'long double' may lose precision [-Wimplicit-int-float-conversion]
38 | s /= n;
| ~~ ^
/Users/ripley/R/Library/Rcpp/include/Rcpp/sugar/functions/mean.h:44:20: warning: implicit conversion from 'R_xlen_t' (aka 'long') to 'long double' may lose precision [-Wimplicit-int-float-conversion]
44 | s += t/n;
| ~^
Activity
Enchufa2 commentedon Jul 30, 2026
This is with gcc 16.1.1 install.log
Enchufa2 commentedon Jul 30, 2026
If we add the warnings from the tests and restrict the search to "may change" (value or the sign of the result), I get
filtered.log
eddelbuettel commentedon Jul 30, 2026
The two 'may change value' are from code borrowed from base R (i.e. in
src/date.cpp). The oneintswitch to assize_t; we could borrow that. The other seems unchanges.All the others go away (under
clang++) when running-Wconversion -Wno-sign-conversionso on balance maybe there is nothing here for us to do.The bigger issue, that is also harder to tackle, is warnings we may tickle in client programs using a different / larger / other part of our headers than the (relatively small) package compilation of
Rcppitself does.Enchufa2 commentedon Jul 31, 2026
Our tests have a pretty decent coverage, so I would say that the filtered log above is a good picture of what others may see.
eddelbuettel commentedon Jul 31, 2026
But test compilation is 'silent' compared to the more visible compilation of files below
src/duringR CMD INSTALL.Enchufa2 commentedon Jul 31, 2026
Ah, you are right, we only see the output for packages that are installed during the tests. I guess we could activate the verbose output for
sourceCppto get the wider picture.eddelbuettel commentedon Sep 3, 2026
Shall we close this one too? BDR still gets different warnings from a different compiler on a different platform (i.e.
clang++on arm64) but for our purposes all of-Wextrafromgccis covered.Enchufa2 commentedon Sep 4, 2026
Let me give it a go to see if there's something useful there.
Enchufa2 commentedon Sep 4, 2026
Independently of what clang may find, activating
-Wconversionwith gcc gives a lot of insight, because it uncovers some potentially buggy places (some real bugs too) where there's ambiguity or we are sloppy with the usage ofintandR_xlen_t. So PR coming, and I think it's worth it leaving this flag activated from now on.eddelbuettel commentedon Sep 4, 2026
Yes. Lossy conversions are bad. My question was really concerned with the open issue as I do not like issue to linger :)