Fix errors resulting from -Wconversion -Wno-sign-conversion
#349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paleolimbot
added a commit
to apache/arrow
that referenced
this pull request
Dec 22, 2023
### Rationale for this change We have failing CRAN checks because this warning occurs on one check machine. ### What changes are included in this PR? Implicit integer casts are made explicit and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers. ### Are these changes tested? This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough `clang` to support the requisite `-W` flags. I tested this locally by adding `PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror` to `Makevars.in`. ### Are there any user-facing changes? No * Closes: #39138 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>
assignUser
pushed a commit
to apache/arrow
that referenced
this pull request
Dec 23, 2023
### Rationale for this change We have failing CRAN checks because this warning occurs on one check machine. ### What changes are included in this PR? Implicit integer casts are made explicit and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers. ### Are these changes tested? This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough `clang` to support the requisite `-W` flags. I tested this locally by adding `PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror` to `Makevars.in`. ### Are there any user-facing changes? No * Closes: #39138 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>
|
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! |
clayburn
pushed a commit
to clayburn/arrow
that referenced
this pull request
Jan 23, 2024
### Rationale for this change We have failing CRAN checks because this warning occurs on one check machine. ### What changes are included in this PR? Implicit integer casts are made explicit and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers. ### Are these changes tested? This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough `clang` to support the requisite `-W` flags. I tested this locally by adding `PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror` to `Makevars.in`. ### Are there any user-facing changes? No * Closes: apache#39138 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>
dgreiss
pushed a commit
to dgreiss/arrow
that referenced
this pull request
Feb 19, 2024
### Rationale for this change We have failing CRAN checks because this warning occurs on one check machine. ### What changes are included in this PR? Implicit integer casts are made explicit and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers. ### Are these changes tested? This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough `clang` to support the requisite `-W` flags. I tested this locally by adding `PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror` to `Makevars.in`. ### Are there any user-facing changes? No * Closes: apache#39138 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>
krlmlr
reviewed
May 1, 2024
Comment on lines
+100
to
104
| if (ISNA(value) && sizeof(T) >= sizeof(int)) { | ||
| return static_cast<T>(NA_INTEGER); | ||
| } else if (!ISNA(value) && is_convertible_without_loss_to_integer(value)) { | ||
| return static_cast<T>(value); | ||
| } |
There was a problem hiding this comment.
This gave me pause. Perhaps:
Suggested change
| if (ISNA(value) && sizeof(T) >= sizeof(int)) { | |
| return static_cast<T>(NA_INTEGER); | |
| } else if (!ISNA(value) && is_convertible_without_loss_to_integer(value)) { | |
| return static_cast<T>(value); | |
| } | |
| if (ISNA(value)) { | |
| if (sizeof(T) >= sizeof(int)) { | |
| return static_cast<T>(NA_INTEGER); | |
| } | |
| // For T of smaller size, can't do better than throw an error | |
| } else if (is_convertible_without_loss_to_integer(value)) { | |
| return static_cast<T>(value); | |
| } |
| if (LOGICAL_ELT(from, 0) == NA_LOGICAL) { | ||
| return NA_INTEGER; | ||
| if (LOGICAL_ELT(from, 0) == NA_LOGICAL && sizeof(T) >= sizeof(int)) { | ||
| return static_cast<T>(NA_INTEGER); | ||
| } |
There was a problem hiding this comment.
Comment what happens in the "else" case? Do we never return value here, is this desired?
|
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
It isn't! I believe that at least |
There was a problem hiding this comment.
I've decided to close this unless CRAN turns these checks back on. Compiling cpp11tests locally with these warnings turned on makes a TON of mostly harmless warnings crop up. Like conversion of size_t to R_xlen_t without an explicit static_cast.
The as_cpp() one here is a bit more "real" but I think at some point we put the burden of using these on the user. Its tough to try and be fully generic and fully safe. It looks like there are other places in as.hpp where we'd also have to make similar changes, so I feel like its better to just keep it all as is for now rather than partially tweak some of these
| @@ -36,7 +36,7 @@ class data_frame : public list { | |||
| return R_NilValue; | |||
| } | |||
|
|
|||
| static int calc_nrow(SEXP x) { | |||
| static R_xlen_t calc_nrow(SEXP x) { | |||
Comment on lines
-20
to
+21
| : data_(safe[Rf_mkCharLenCE](data.c_str(), data.size(), CE_UTF8)) {} | ||
| : data_( | ||
| safe[Rf_mkCharLenCE](data.c_str(), static_cast<int>(data.size()), CE_UTF8)) {} |
| @@ -76,7 +76,7 @@ class sexp { | |||
|
|
|||
| operator SEXP() const { return data_; } | |||
| operator double() const { return REAL_ELT(data_, 0); } | |||
| operator size_t() const { return REAL_ELT(data_, 0); } | |||
| operator size_t() const { return static_cast<size_t>(REAL_ELT(data_, 0)); } | |||
There was a problem hiding this comment.
Trying to remove these in #390
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 apache/arrow#39250 ), but a few are in the cpp11 headers.The biggest change is for
as_cpp<>(), templates for which we instantiate in arrow forint64_t, anduint8_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: