-
Notifications
You must be signed in to change notification settings - Fork 4k
[c++] default to C++17 for most builds #7016
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
Conversation
|
There are 2 CI jobs failing, both using older build toolchains.
Those are both toolchains we've previously discussed dropping. Let's drop those and then make C++17 the default in LightGBM.
|
StrikerRUS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also update wording here?
Lines 85 to 91 in 061f59a
| elseif(MSVC) | |
| if(MSVC_VERSION LESS 1900) | |
| message( | |
| FATAL_ERROR | |
| "The compiler ${CMAKE_CXX_COMPILER} doesn't support required C++11 features. Please use a newer MSVC." | |
| ) | |
| endif() |
Not sure, but should we use CXX17 here now?
Line 77 in 061f59a
| CXX11=g++-8 |
I just checked: we can safely remove -build/c++11 rule from filter for cpplint.
LightGBM/.pre-commit-config.yaml
Line 31 in 061f59a
| - --filter=-build/c++11,-build/include_subdir,-build/include_what_you_use,-build/header_guard,-whitespace/indent_namespace,-whitespace/line_length |
| FATAL_ERROR | ||
| "The compiler ${CMAKE_CXX_COMPILER} doesn't support required C++11 features. Please use a newer MSVC." | ||
| ) | ||
| message(FATAL_ERROR "Insufficient MSVC version (${MSVC_VERSION})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous message was probably technically still true, but this rewording is a bit simpler and is consistent with the other enforce-a-minimum-compiler-version errors.
It's enough for users to be told that the version of a compiler they're using is older than what LightGBM supports... saying for MSVC that that's because of "C++11 features" doesn't add much information.
| CC17=gcc-14 | ||
| CXX=g++-14 | ||
| CXX17=g++-14 | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested all of this on a Mac with R 4.4.2 tonight, confirmed it worked as expected.
|
All great suggestions, thanks @StrikerRUS ! I looked for more such things like this and didn't find any: git grep -E -i 'c\+\+.*11'I've addressed the 3 you mentioned, left inline comments explaining those choices. |
StrikerRUS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for working on this!
|
It's good that we did this, and that it'll be in the next release... just noticed that C++11/C++14 support was recently removed in R-devel!! |
Follow-up to #7013
@h-vetinari made a good point there that we probably could safely default to C++17 in most builds here: #7013 (comment)
See the notes on that PR... C++17 has been supported for 6+ years in most compilers by now, so I don't think we lose much portability by doing this. And this should make things easier for upgrading dependencies (for example, linking to newer CUDA Toolkit, Boost, etc.).
Notes for Reviewers
There will still be one use of C++11 leftFor as long as the R package supports R 3.x, the core source code (excluding anything to do with GPUs or MPI) still needs to be compilable with C++11:LightGBM/R-package/configure.win
Lines 10 to 20 in de4c88f
Moving the minimum R version to R 4.x would eliminate that. R 4.0 was released 5.5 years ago (April 2020) so it's probably been long enough... but let's not make that a part of this PR. I'll open a separate RFC issue.update: R minimum version moved to 4.0 in #7038