[RFC] Support C++17 standard?

[jameslamb]

Summary

I'm seeking comments on the following questions

1. Should LightGBM support the C++17 standard for all components?

2. If yes, should it continue to default to the C++11 standard?

By "support", I mean "have at least one CI job testing LightGBM compiled with C++17 standard on Mac (clang), Linux (non-CUDA w/ gcc, CUDA with nvcc), and Windows (MINGW and MSVC)".

Motivation

CRAN might force the R package to upgrade to that standard soon (see the proposal in #5690).

Beyond just R, though, there are other new language features LightGBM might benefit from in C++17

• https://en.cppreference.com/w/cpp/17
• https://lhcb.github.io/developkit-lessons/first-development-steps/05c-cpp17.html
• https://www.oreilly.com/content/c17-upgrades-you-should-be-using-in-your-code/

Can LightGBM's vendored dependencies be compiled with the C++17 standard?

Yes! See the passing CI jobs in #5690, plus the following links.

• Boost: C++03 (via backports), C++11 (full), with support for some C++14 and C++17 features (docs link)
• Eigen: C++17 (docs link, code link)
• fast_double_parser: C++11 (code link)
• fmt: C++11, C++14, C++17, C++20 (docs link)

If C++17 became the default, what range of compiler versions would LightGBM work with?

• gcc >= 5 (October 2017, supporting docs)
• clang >= 5 (September 2017, supporting docs)
• MSVC >= 2019 (April 2019, supporting docs)
• nvcc in CUDA 11+ (May 2020, supporting docs)

Reviewers

Tagging in maintainers and other contributors who I feel might have an opinion on this, and who are probably more knowledgeable about this than me.

@shiyu1994 @guolinke @StrikerRUS @jmoralez @btrotta @Laurae2 @huanzhang12 @imatiach-msft @AlbertoEAF @lemire @trivialfis @hcho3 @ChipKerchner @cyfdecyf

Thanks all for your time and consideration!

[trivialfis]

Hi, thank you for tagging me. Having a c++17 test on CI is different from using c++17 in the codebase. It's beneficial to have c++-17 if you use lots of meta-programming or rely on libraries that do. Also, if you plan to have some data structure like std::vector to manage both CPU and GPU memory, the new runtime allocator can be helpful.

Out of curiosity, in https://github.com/microsoft/LightGBM/pull/5690 , how do you submit a CRAN package in the future to support both old-rel, current release, and devel?

[jameslamb]

Having a c++17 test on CI is different from using c++17 in the codebase.

Definitely! Thanks for your detailed answer. I'm specifically proposing the CI jobs as a minimal commitment that LightGBM won't use use earlier language features that were removed as of C++17 (like std::random_shuffle).

in https://github.com/microsoft/LightGBM/pull/5690 , how do you submit a CRAN package in the future to support both old-rel, current release, and devel?

Based on https://cran.r-project.org/web/checks/check_flavors.html, I believe all CRAN check flavors are using versions of R + compilers that support C++17.

Even Windows old-rel, usually the furthest behind in my experience, is using R 4.1.3 (https://www.r-project.org/nosvn/R.check/r-oldrel-windows-ix86+x86_64/lightgbm-00check.html) and gcc 8 (via MINGW).

According to https://github.com/search?q=org%3Acran+%22C%2B%2B17%22+path%3A**%2FDESCRIPTION&type=code, there are already 33 libraries on CRAN using SystemRequirements: C++17. Including {arrow}, the R package for Apache Arrow (https://cran.r-project.org/web/packages/arrow/index.html).

[lemire]

All my new projects will require C++17. It is a significant step forward for C++... It is time to move.

[AlbertoEAF]

Sounds great! :)

On Sun, 29 Jan 2023, 01:55 Daniel Lemire, @.***> wrote:

All my new projects will require C++17. It is a significant step forward for C++... It is time to move.

— Reply to this email directly, view it on GitHub https://github.com/microsoft/LightGBM/issues/5691#issuecomment-1407534993, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6K4MPL7QQRQZEMZSC4MXLWUXEXNANCNFSM6AAAAAAUJPR2LA . You are receiving this because you were mentioned.Message ID: @.***>

[hcho3]

Thanks for pinging me. Good to know that C++17 is being adopted widely. XGBoost project should move to C++17 standard as well. (*)

(*) XGBoost is actually using C++17 when a particular plugin is enabled (RMM plugin), so it's already tested with C++17

[shiyu1994]

I do recognize that some features of c++ 17 is quite useful when developing. For example, we may want to use the constexpr if to move the cost for executing some if statements with template parameters to compile time, if we want to pursue efficiency.

We may have some macros in the code to switch between c++11 and c++17, if we want to move to c++17 gradually without immediately drop the support with c++11.

[jameslamb]

For sure! At a minimum, I think we should:

• keep C++11 as the default
• have a couple CI jobs testing with C++17 standard, to at least say "we won't rely on any language features that were dropped by C++17"

But I think there's a good case for making C++17 the minimum standard LightGBM supports. Given that it's been out for so long and such a wide range of compilers support it.

[trivialfis]

We just got a note from cran pre-test:

* checking C++ specification ... NOTE
  Specified C++14: please drop specification unless essential

It seems we are not encouraged to specify the version of c++ std

[lemire]

It seems we are not encouraged to specify the version of c++ std

That's not how I read the note you reproduce. The default support (e.g., R 4.1.0) is C++14 I believe (please check). Hence, requiring C++14 is unnecessary and potentially harmful long term.

[jameslamb]

I know there was a lengthy discussion about this in r-pkg-devel recently.

I need to re-read it before I offer an opinion about its implications for projects like LightGBM and XGBoost, but here's the thread for those interested: https://stat.ethz.ch/pipermail/r-package-devel/2023q1/008870.html

[lemire]

The relevant documentation is here:

https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Using-C_002b_002b-code

[celestinoxp]

the new visual studio speaks in c++23, isn't that something to take into account too? https://devblogs.microsoft.com/cppblog/visual-studio-17-5-for-cpp-devs/#standards-conformance

[jameslamb]

I've just merged #5690, requesting C++17 in the CRAN build of the R package (with a test in configure.win falling back to C++11 if C++17 isn't supported, for R3.6 + Rtools35).

So LightGBM will now have a mix of tests of C++11 compatibility and C++17 compatibility in its CI.

the new visual studio speaks in c++23, isn't that something to take into account too?

Thanks, yes it's a related concern. I'm planning to add a CI job testing with MSVC + /std++latest (https://github.com/microsoft/LightGBM/pull/5500#issuecomment-1367535385), so we get early feedback of incompatibility with new standards.

[jameslamb]

This can be closed. The CRAN-style source distribution of the R package has been using the C++17 standard across a wide range of operating systems and compilers for almost a year (#5690).

Thanks to everyone who contributed to this discussion!