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

fix many cpp lint errors #2426

Merged
merged 6 commits into from Sep 22, 2019
Merged

fix many cpp lint errors #2426

merged 6 commits into from Sep 22, 2019

Conversation

guolinke
Copy link
Collaborator

refer to #1990

@guolinke
Copy link
Collaborator Author

BTW, this will have some conflicts with other PRs.
@StrikerRUS I think we should merge this ASAP.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Wow, amazing! 🎉

I'm only afraid of reinterpret_cast<void*>: #2064 (comment)

@guolinke
Copy link
Collaborator Author

@StrikerRUS yeah, but it seems the reinterpret_cast is the only way, and it actually equals to c style cast.

@guolinke
Copy link
Collaborator Author

@StrikerRUS BTW, as json11 is not maintained by us, we can disable the lint check over it.

@StrikerRUS
Copy link
Collaborator

@guolinke Yes, but why?.. As it's not a "submodule" (as compute) it falls into situation when it's maintained by no one. I think it's not right. Seems that the license allows to modify it. I remember there were some dangerous warnings from cpplint for json11.

@guolinke
Copy link
Collaborator Author

Okay, I think we won't modify it, so we can keep it as it is. However, if these warnings are easy to fix, we could fix it directly.

@guolinke guolinke merged commit f1a1486 into master Sep 22, 2019
@StrikerRUS StrikerRUS deleted the cpp-lint-fix branch September 22, 2019 12:52
@StrikerRUS StrikerRUS mentioned this pull request Mar 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants