Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[R] Compile with -Wconversion on clang15 results in compiler warnings #39138

Closed
paleolimbot opened this issue Dec 8, 2023 · 4 comments · Fixed by #39250
Closed

[R] Compile with -Wconversion on clang15 results in compiler warnings #39138

paleolimbot opened this issue Dec 8, 2023 · 4 comments · Fixed by #39250
Assignees
Milestone

Comments

@paleolimbot
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

As identified by the CRAN M1 check: https://www.stats.ox.ac.uk/pub/bdr/M1mac/arrow.log

A few are in cpp11 headers:

In file included from /Users/ripley/R/Library/cpp11/include/cpp11/as.hpp:11:
/Users/ripley/R/Library/cpp11/include/cpp11/protect.hpp:130:29: warning: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Wshorten-64-to-32]
  return std::forward<F>(f)(std::get<I>(std::move(a))...);
         ~~~                ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ripley/R/Library/cpp11/include/cpp11/protect.hpp:136:10: note: in instantiation of function template specialization 'cpp11::detail::apply<SEXPREC *(*&)(const char *, int, cetype_t), const char *&&, unsigned long &&, cetype_t &&, 0UL, 1UL, 2UL>' requested here
  return apply(std::forward<F>(f), std::move(a), make_index_sequence<sizeof...(Aref)>{});
         ^
/Users/ripley/R/Library/cpp11/include/cpp11/protect.hpp:149:12: note: in instantiation of function template specialization 'cpp11::detail::apply<SEXPREC *(*&)(const char *, int, cetype_t), const char *&&, unsigned long &&, cetype_t &&>' requested here
    return apply(ptr_, std::move(arefs_));
           ^
/Users/ripley/R/Library/cpp11/include/cpp11/protect.hpp:56:16: note: in instantiation of member function 'cpp11::detail::closure<SEXPREC *(const char *, int, cetype_t), const char *&&, unsigned long &&, cetype_t &&>::operator()' requested here
        return static_cast<Fun&&>(*callback)();
               ^
/Users/ripley/R/Library/cpp11/include/cpp11/protect.hpp:163:14: note: in instantiation of function template specialization 'cpp11::unwind_protect<cpp11::detail::closure<SEXPREC *(const char *, int, cetype_t), const char *&&, unsigned long &&, cetype_t &&>, void>' requested here
      return unwind_protect(
             ^
/Users/ripley/R/Library/cpp11/include/cpp11/r_string.hpp:20:35: note: in instantiation of function template specialization 'cpp11::protect::function<SEXPREC *(const char *, int, cetype_t)>::operator()<const char *, unsigned long, cetype_t>' requested here
      : data_(safe[Rf_mkCharLenCE](data.c_str(), data.size(), CE_UTF8)) {}
                                  ^
In file included from table.cpp:18:
././arrow_types.h:206:16: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
        lambda(j, x_i[k], names_x_i[k]);
        ~~~~~~ ^
././arrow_types.h:218:3: note: in instantiation of function template specialization 'arrow::r::TraverseDots<(lambda at ././arrow_types.h:217:14)>' requested here
  TraverseDots(dots, num_fields, set);
  ^
././arrow_types.h:209:14: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
      lambda(j, dots[i], name_i);
      ~~~~~~ ^
././arrow_types.h:206:16: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
        lambda(j, x_i[k], names_x_i[k]);
        ~~~~~~ ^
table.cpp:182:13: note: in instantiation of function template specialization 'arrow::r::TraverseDots<(lambda at table.cpp:170:28)>' requested here
  arrow::r::TraverseDots(lst, num_fields, extract_one_field);
            ^
In file included from table.cpp:18:
././arrow_types.h:209:14: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
      lambda(j, dots[i], name_i);
      ~~~~~~ ^
././arrow_types.h:206:16: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
        lambda(j, x_i[k], names_x_i[k]);
        ~~~~~~ ^
table.cpp:213:13: note: in instantiation of function template specialization 'arrow::r::TraverseDots<(lambda at table.cpp:203:31)>' requested here
  arrow::r::TraverseDots(lst, num_fields, extract_one_metadata);
            ^
In file included from table.cpp:18:
././arrow_types.h:209:14: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
      lambda(j, dots[i], name_i);
      ~~~~~~ ^
22 warnings generated.

In file included from /Users/ripley/R/Library/cpp11/include/cpp11.hpp:5:
/Users/ripley/R/Library/cpp11/include/cpp11/as.hpp:104:16: warning: implicit conversion turns floating-point number into integer: 'double' to 'enable_if_integral<long long, long long>' (aka 'long long') [-Wfloat-conversion]
        return value;
        ~~~~~~ ^~~~~
io.cpp:238:25: note: in instantiation of function template specialization 'cpp11::as_cpp<long long>' requested here
          return cpp11::as_cpp<int64_t>(result);
                        ^
15 warnings generated.

In file included from json.cpp:18:
In file included from ././arrow_types.h:22:
In file included from ././arrow_cpp11.h:25:
In file included from /Users/ripley/R/Library/cpp11/include/cpp11.hpp:7:
In file included from /Users/ripley/R/Library/cpp11/include/cpp11/data_frame.hpp:12:
In file included from /Users/ripley/R/Library/cpp11/include/cpp11/list.hpp:7:
In file included from /Users/ripley/R/Library/cpp11/include/cpp11/named_arg.hpp:9:
/Users/ripley/R/Library/cpp11/include/cpp11/sexp.hpp:79:36: warning: implicit conversion turns floating-point number into integer: 'double' to 'size_t' (aka 'unsigned long') [-Wfloat-conversion]
  operator size_t() const { return REAL_ELT(data_, 0); }
                            ~~~~~~ ^~~~~~~~~~~~~~~~~~
In file included from json.cpp:18:
In file included from ././arrow_types.h:22:
In file included from ././arrow_cpp11.h:25:
In file included from /Users/ripley/R/Library/cpp11/include/cpp11.hpp:7:
/Users/ripley/R/Library/cpp11/include/cpp11/data_frame.hpp:48:14: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
      return Rf_xlength(nms);
      ~~~~~~ ^~~~~~~~~~~~~~~
/Users/ripley/R/Library/cpp11/include/cpp11/data_frame.hpp:55:12: warning: implicit conversion loses integer precision: 'R_xlen_t' (aka 'long') to 'int' [-Wshorten-64-to-32]
    return Rf_xlength(VECTOR_ELT(x, 0));
    ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

A few are in Arrow C++ headers:

In file included from RTasks.cpp:18:
In file included from ././r_task_group.h:20:
In file included from /Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/parallel.h:25:
In file included from /Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/thread_pool.h:32:
In file included from /Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/future.h:32:
In file included from /Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/type_traits.h:26:
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/bit_util.h:338:84: warning: implicit conversion loses integer precision: 'int' to 'unsigned char' [-Wimplicit-int-conversion]
  return (static_cast<Word>(i < sizeof(Word) * 8) << (i & (sizeof(Word) * 8 - 1))) - 1;
  ~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/bit_util.h:340:15: note: in instantiation of function template specialization 'arrow::bit_util::PrecedingWordBitmask<unsigned char>' requested here
static_assert(PrecedingWordBitmask<uint8_t>(0) == 0x00, "");
              ^
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/bit_util.h:338:84: warning: implicit conversion loses integer precision: 'int' to 'unsigned short' [-Wimplicit-int-conversion]
  return (static_cast<Word>(i < sizeof(Word) * 8) << (i & (sizeof(Word) * 8 - 1))) - 1;
  ~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/bit_util.h:343:15: note: in instantiation of function template specialization 'arrow::bit_util::PrecedingWordBitmask<unsigned short>' requested here
static_assert(PrecedingWordBitmask<uint16_t>(8) == 0x00ff, "");
              ^
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/bit_util.h:361:64: warning: implicit conversion loses integer precision: 'uint32_t' (aka 'unsigned int') to 'uint8_t' (aka 'unsigned char') [-Wimplicit-int-conversion]
              values[4] << 4 | values[5] << 5 | values[6] << 6 | values[7] << 7);
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
3 warnings generated.

In file included from io.cpp:23:
In file included from /Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/buffer_builder.h:30:
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/util/bitmap_generate.h:95:35: warning: implicit conversion loses integer precision: 'int' to 'uint8_t' (aka 'unsigned char') [-Wimplicit-int-conversion]
              out_results[6] << 6 | out_results[7] << 7);
              ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
/Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/buffer_builder.h:369:15: note: in instantiation of function template specialization 'arrow::internal::GenerateBitsUnrolled<(lambda at /Users/ripley/R/packages/tests-devel/arrow/libarrow/arrow-14.0.0/include/arrow/buffer_builder.h:369:79)>' requested here
    internal::GenerateBitsUnrolled(mutable_data(), bit_length_, num_elements, [&] {
   

Component(s)

R

@assignUser assignUser added this to the 14.0.2 milestone Dec 8, 2023
@assignUser
Copy link
Member

assignUser commented Dec 8, 2023

That's not a vendored cpp11 header right? So nothing we can do about it? Thanks for picking this up!

@paleolimbot
Copy link
Member Author

So nothing we can do about it?

I can PR a fix if nobody has gotten there in a few days...some of those may be us, too and we just instantiate templates that otherwise wouldn't have been touched.

@raulcd
Copy link
Member

raulcd commented Dec 9, 2023

is this a blocker for 14.0.2?

@paleolimbot
Copy link
Member Author

It will be very difficult (i.e., be a lot of work for Jacob...basically inventing a way to patch Arrow C++'s headers in the R package) to fix the CRAN status without the Arrow C++ portion of this included in 14.0.2. It might be more appropriate to separate the Arrow C++ half of this, which is very small, and consider that a blocker.

paleolimbot added a commit that referenced this issue Dec 12, 2023
…-conversion` in public headers (#39186)

### Rationale for this change

The R package has a warning from CRAN to fix a failure to compile with `-Wconversion -Wno-sign-conversion -Werror`. Some of these errors we control and can patch easily; however, the ones in the Arrow C++ portion are more difficult to work around (hence the separate PR). See #39138 for all reported errors (including those in just the R package).

### What changes are included in this PR?

The requisite `static_cast<>()`s were added to silence the warnings.

### Are these changes tested?

By existing tests. We may add a future R nightly job that runs with these warning flags.

### Are there any user-facing changes?

No
* Closes: #39185

Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
mapleFU pushed a commit to mapleFU/arrow that referenced this issue Dec 13, 2023
…o-sign-conversion` in public headers (apache#39186)

### Rationale for this change

The R package has a warning from CRAN to fix a failure to compile with `-Wconversion -Wno-sign-conversion -Werror`. Some of these errors we control and can patch easily; however, the ones in the Arrow C++ portion are more difficult to work around (hence the separate PR). See apache#39138 for all reported errors (including those in just the R package).

### What changes are included in this PR?

The requisite `static_cast<>()`s were added to silence the warnings.

### Are these changes tested?

By existing tests. We may add a future R nightly job that runs with these warning flags.

### Are there any user-facing changes?

No
* Closes: apache#39185

Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@raulcd raulcd modified the milestones: 14.0.2, 15.0.0 Dec 13, 2023
paleolimbot added a commit that referenced this issue 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 <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
assignUser pushed a commit that referenced this issue Dec 23, 2023
…-conversion` in public headers (#39186)

### Rationale for this change

The R package has a warning from CRAN to fix a failure to compile with `-Wconversion -Wno-sign-conversion -Werror`. Some of these errors we control and can patch easily; however, the ones in the Arrow C++ portion are more difficult to work around (hence the separate PR). See #39138 for all reported errors (including those in just the R package).

### What changes are included in this PR?

The requisite `static_cast<>()`s were added to silence the warnings.

### Are these changes tested?

By existing tests. We may add a future R nightly job that runs with these warning flags.

### Are there any user-facing changes?

No
* Closes: #39185

Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
assignUser pushed a commit that referenced this issue 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 <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
clayburn pushed a commit to clayburn/arrow that referenced this issue Jan 23, 2024
…o-sign-conversion` in public headers (apache#39186)

### Rationale for this change

The R package has a warning from CRAN to fix a failure to compile with `-Wconversion -Wno-sign-conversion -Werror`. Some of these errors we control and can patch easily; however, the ones in the Arrow C++ portion are more difficult to work around (hence the separate PR). See apache#39138 for all reported errors (including those in just the R package).

### What changes are included in this PR?

The requisite `static_cast<>()`s were added to silence the warnings.

### Are these changes tested?

By existing tests. We may add a future R nightly job that runs with these warning flags.

### Are there any user-facing changes?

No
* Closes: apache#39185

Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
clayburn pushed a commit to clayburn/arrow that referenced this issue 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 <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…o-sign-conversion` in public headers (apache#39186)

### Rationale for this change

The R package has a warning from CRAN to fix a failure to compile with `-Wconversion -Wno-sign-conversion -Werror`. Some of these errors we control and can patch easily; however, the ones in the Arrow C++ portion are more difficult to work around (hence the separate PR). See apache#39138 for all reported errors (including those in just the R package).

### What changes are included in this PR?

The requisite `static_cast<>()`s were added to silence the warnings.

### Are these changes tested?

By existing tests. We may add a future R nightly job that runs with these warning flags.

### Are there any user-facing changes?

No
* Closes: apache#39185

Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue 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 <dewey@fishandwhistle.net>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants