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

Support compiling with std:c++latest using MSVC (fixes #5150) #5500

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

QuellaZhang
Copy link

@QuellaZhang QuellaZhang commented Sep 23, 2022

Fixes #5150.

@jameslamb jameslamb changed the title [MSVC][std:c++latest] Fix compiler error [MSVC][std:c++latest] Fix compiler error (fixes #5150) Oct 11, 2022
@jameslamb jameslamb self-requested a review October 11, 2022 04:12
Copy link
Collaborator

@jameslamb jameslamb left a 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.

@QuellaZhang
Copy link
Author

@jameslamb, I have updated the branch.

@jameslamb jameslamb changed the title [MSVC][std:c++latest] Fix compiler error (fixes #5150) Support compiling with std:c++latest using MSVC (fixes #5150) Oct 18, 2022
@jameslamb jameslamb self-requested a review October 18, 2022 03:25
Copy link
Collaborator

@jameslamb jameslamb left a 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.

@guolinke
Copy link
Collaborator

In my opinion, as long as the changes do not break the C++11, I am okay with it.

@StrikerRUS
Copy link
Collaborator

cc @AlbertoEAF

@StrikerRUS
Copy link
Collaborator

@jameslamb

I think a new CI job using this option should be added as part of this PR

WDYT about using std:c++latest flag in a part of already existing our CI jobs to save CI time? For example, that flag can be added in R-package CI job that runs against MSVC-2022

# Visual Studio 2022
- os: windows-2022
task: r-package
compiler: MSVC
toolchain: MSVC
r_version: 4.2
build_type: cmake

@jameslamb
Copy link
Collaborator

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?

@jameslamb
Copy link
Collaborator

@QuellaZhang I'm ready to work with you on getting this PR merged.

Are you interested in trying to update it to use std:c++latest on the R package's MSVC job as @StrikerRUS suggested? If you don't have the time or interested (or if I don't hear back from you in the next few weeks), I'll open a separate PR with these changes + the testing changes.

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.

[MSVC][std:c++latest] LightGBM failed to build with /std:c++latest on MSVC
4 participants