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

Add a dummy reference to prevent unused variable warning if there is … #5196

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

yang-g
Copy link
Member

@yang-g yang-g commented Feb 11, 2016

…no rpc method defined.

@grpc-kokoro
Copy link

Can one of the admins verify this patch?

1 similar comment
@grpc-kokoro
Copy link

Can one of the admins verify this patch?

@vjpai
Copy link
Member

vjpai commented Feb 11, 2016

Do we have a test that covers this case? Can be added later, but please file an issue to do so if that's the route taken.

@yang-g
Copy link
Member Author

yang-g commented Feb 11, 2016

Currently we do not have unit tests for code generation. See #1225 for discussions.

@vjpai
Copy link
Member

vjpai commented Feb 11, 2016

Oh no, I wasn't asking for a unit test. That's it's own beast (and I also don't like the golden-file testing approach). I was asking if any of our existing protos in open-source actually exhibit this behavior (no rpc method defined). I think the answer is no, and it would be nice to have coverage of that case to make sure that nobody strips out this reference in the future thinking, "Hmm, useless void ref" . I'm fine with an issue filed even if the proto isn't filled in now.

@vjpai
Copy link
Member

vjpai commented Feb 11, 2016

All C++ tests pass, so I declare this PR mergeworthy.

vjpai added a commit that referenced this pull request Feb 11, 2016
Add a dummy reference to prevent unused variable warning if there is …
@vjpai vjpai merged commit bfa6eb9 into grpc:master Feb 11, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2019
@lock lock bot unassigned dgquintas Jan 28, 2019
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.

5 participants