search

r-lib / cpp11

cpp11 helps you to interact with R objects using C++ code.
https://cpp11.r-lib.org/
Other
193 stars 46 forks source link

Fix errors resulting from `-Wconversion -Wno-sign-conversion` #349

Open paleolimbot opened 6 months ago

paleolimbot commented 6 months ago

The M1 runner apparently runs checks with -Wconversion -Wno-sign-conversion -Werror, which has caused the arrow package to have a very long list of compiler warnings/build failures on the package check page ( https://www.stats.ox.ac.uk/pub/bdr/M1mac/arrow.log ). Most of these are in the arrow package (fixed by https://github.com/apache/arrow/pull/39250 ), but a few are in the cpp11 headers.

The biggest change is for as_cpp<>(), templates for which we instantiate in arrow for int64_t, and uint8_t, both of which interact poorly with various pieces of that logic. I'm happy to update this and add some tests but I wanted to check first to see if there's consensus on what should actually happen. A few cases that were exposed by these warnings are:

as_cpp<int64_t>(NA_INTEGER); // static_cast<int64_t>(NA_INTEGER)?
as_cpp<int8_t>(NA_REAL); // error?
as_cpp<int8_t>(NA_INTEGER); // error?
as_cpp<int>(static_cast<double>(INT_MAX) + 1); // error?
paleolimbot commented 6 months ago

The warnings have gone away from the CRAN check page; however, I'm still happy to tighten up + test these changes if they're welcome!

paleolimbot commented 2 months ago

Thanks for taking a look! I will circle back to this when I get a break from my current project...you've raised excellent points and it would be nice to tighten up the automatic conversion for integral types other than just int.

is it still necessary?

It isn't! I believe that at least -Wconversion and/or -Wno-sign-conversion was turned off in the MacOS M1 runner, and thus the CRAN warning went away.