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

fixed cpplint errors and disable warning only for VS #2888

Merged
merged 2 commits into from
Mar 11, 2020
Merged

Conversation

StrikerRUS
Copy link
Collaborator

Refer to #1990 and #2883.

@@ -76,8 +76,10 @@ const int kAlignedSize = 32;
#define SIZE_ALIGNED(t) ((t) + kAlignedSize - 1) / kAlignedSize * kAlignedSize

// Refer to https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4127?view=vs-2019
#pragma warning(disable : 4127)
#ifdef _MSC_VER
#pragma warning(disable : 4127)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some other warning disable could use the msc ver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guolinke Sorry, didn't get it. Can you please elaborate?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I means, there are some other #pragma warning(disable : xxxx), which are not inside #ifdef _MSC_VER.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it! I thought that the reference to VS in the corresponding comment means that it is actually needed only for VS. Like in

#ifdef _MSC_VER
#pragma warning(disable: 4068) // disable unknown pragma warning
#endif

And I was aware only about that pragma warning in openmp_wrapper code. Should I search for others and wrap them as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I remember there are some in c_api.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@StrikerRUS StrikerRUS merged commit bd10918 into master Mar 11, 2020
@StrikerRUS StrikerRUS deleted the cpplint branch March 11, 2020 11:45
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants