-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Support compiling with std:c++latest
using MSVC (fixes #5150)
#5500
base: master
Are you sure you want to change the base?
Conversation
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.
Sorry for the long delay, we had some CI issues and then needed to focus on a time-sensitive release.
Those things are resolved, and I'm ready to help move this forward. Could you please merge the latest changes from master
into this branch? Then we'll be able to provide more of a review.
Thanks very much for your patience, and sorry for the inconvenience.
@jameslamb, I have updated the branch. |
std:c++latest
using MSVC (fixes #5150)
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.
Thanks for this!
Based on successful CI jobs, this change looks ok to me...but the passing CI jobs don't prove that it fixes compilation with MSVC using std:c++latest
. As I mentioned, LightGBM is using the C++11 standard everywhere: #5150 (comment).
@guolinke @shiyu1994 Are you comfortable with LightGBM committing to continue supporting std:c++latest
, as new C++ standards come out and the specific spec referenced by latest
changes? (documentation). If so, I think a new CI job using this option should be added as part of this PR, to ensure that future LightGBM doesn't break this support.
If not, and you'd instead prefer to instead just accept targeted patches like this based on reported issues from users, then it's probably ok to not add a new CI job at this time.
In my opinion, as long as the changes do not break the C++11, I am okay with it. |
cc @AlbertoEAF |
WDYT about using LightGBM/.github/workflows/r_package.yml Lines 95 to 101 in 0c0eb2a
|
yeah @StrikerRUS , I'm ok with that! I think that, based on https://learn.microsoft.com/en-us/cpp/build/reference/cl-environment-variables?view=msvc-170, this could be done by setting $env:CL = "/std:c++latest" somewhere. Does that seem right to you? |
@QuellaZhang I'm ready to work with you on getting this PR merged. Are you interested in trying to update it to use |
Fixes #5150.