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 sign-comparison warnings and add a test for that. #1829

Merged
merged 2 commits into from
Jul 23, 2016

Conversation

xfxyjwf
Copy link
Contributor

@xfxyjwf xfxyjwf commented Jul 22, 2016

Fixes #1813

Also added a test to ensure public header files and protobuf generated code doesn't have these warnings. This test doesn't cover protobuf's own implementation code. We may want to update all of protobuf's code to be free of this warning in the future but right now there are just too many of them and require more work to cleanup.

xfxyjwf added 2 commits July 22, 2016 14:15
grpc build treates them as errors and such issues (protobuf change
breaks grpc) has been reported repeatedly. For example:
  protocolbuffers#1813

Change-Id: I077c4557cf3effd5195f88802c38999b884edc30
@xfxyjwf
Copy link
Contributor Author

xfxyjwf commented Jul 22, 2016

@pherl to review

@@ -122,7 +124,6 @@ nobase_include_HEADERS = \
google/protobuf/reflection.h \
google/protobuf/reflection_ops.h \
google/protobuf/repeated_field.h \
google/protobuf/repeated_field_reflection.h \
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's deleted internally, but the deletion is lost from internal to opensource integration. See http://cl/71942508

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops, wrong cl number. Should be this one: http://cl/86669259

@liujisi
Copy link
Contributor

liujisi commented Jul 22, 2016

LGTM

@xfxyjwf xfxyjwf merged commit 1b3796c into protocolbuffers:master Jul 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants