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

[c++] include <cstdint> wherever uint8_t is used #6736

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Conversation

jameslamb
Copy link
Collaborator

Earlier today, I got an email from Prof. Brian Ripley (from CRAN):

subject: CRAN packages missing <cstint> header

body: Current snapshots of g++ 15 are complaining that you have omitted the header . Please follow the advice in the log at your next update. (We would contact you again if this persists when a release date for GCC 15 is known, but they tend to give only a week's notice.)

Linking to these logs for a build of lightgbm with gcc-15: https://www.stats.ox.ac.uk/pub/bdr/gcc15/lightgbm.out

It contains compilation errors like this:

io/json11.cpp: In function 'void json11_internal_lightgbm::dump(const std::string&, std::string*)':
io/json11.cpp:93:28: error: 'uint8_t' does not name a type
   93 |     } else if (static_cast<uint8_t>(ch) <= 0x1f) {
      |                            ^~~~~~~
io/json11.cpp:26:1: note: 'uint8_t' is defined in header '<cstdint>'; this is probably fixable by adding '#include <cstdint>'

From the gcc docs (https://gcc.gnu.org/gcc-15/porting_to.html):

Some C++ Standard Library headers have been changed to no longer include other headers that were being used internally by the library. As such, C++ programs that used standard library components without including the right headers will no longer compile.

In particular, the following header is used less widely within libstdc++ and may need to be included explicitly when compiling with GCC 15:

  • <cstdint> (for std::int8_t, std::int32_t , etc.)

There are more notes on this at https://gcc.gnu.org/gcc-15/porting_to.html

Notes for Reviewers

CI job?

I'm not sure how to get a development version of GCC 15 here in CI, to confirm that this is working. https://r-hub.github.io/containers/, for example, does not have a gcc-15 image yet.

If this PR passes all CI (which tests a wide range of compilers and operating systems), I think we should merge it and include it in the next release. Hopefully that will be sufficient.

We can test against gcc-15 as a separate effort.

@jameslamb jameslamb changed the title WIP: [c++] include <cstdint> wherever uint8_t is used [c++] include <cstdint> wherever uint8_t is used Dec 3, 2024
@jameslamb jameslamb marked this pull request as ready for review December 3, 2024 23:12
@StrikerRUS StrikerRUS merged commit d4d6c87 into master Dec 5, 2024
48 checks passed
@StrikerRUS StrikerRUS deleted the support-gcc15 branch December 5, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants