Skip to content

context/core/netty: Add @CheckReturnValue to Context - #4430

Merged
dapengzhang0 merged 2 commits into
grpc:masterfrom
dapengzhang0:contextcheckreturn
May 15, 2018
Merged

context/core/netty: Add @CheckReturnValue to Context#4430
dapengzhang0 merged 2 commits into
grpc:masterfrom
dapengzhang0:contextcheckreturn

Conversation

@dapengzhang0

@dapengzhang0 dapengzhang0 commented May 3, 2018

Copy link
Copy Markdown
Contributor

This change is Reviewable

@dapengzhang0
dapengzhang0 force-pushed the contextcheckreturn branch from 4f731ec to 42b82b5 Compare May 3, 2018 17:01
@carl-mastrangelo

Copy link
Copy Markdown
Contributor

This needs to be check with an internal presubmit first.

@dapengzhang0

dapengzhang0 commented May 3, 2018

Copy link
Copy Markdown
Contributor Author

This needs to be check with an internal presubmit first.

Yes, this change is scary.

@dapengzhang0

Copy link
Copy Markdown
Contributor Author

The internal presubmit (CL/195275931) passed.

@carl-mastrangelo

Copy link
Copy Markdown
Contributor

One other concern: this package has traditionally not had any dependencies (including DoNotMock). @adriancole I recall you being involved in the discussion around deps of Context. Would taking a compile time dependency on ErrorProne annotations be feasible?

@dapengzhang0

Copy link
Copy Markdown
Contributor Author

One other concern: this package has traditionally not had any dependencies (including DoNotMock). @adriancole I recall you being involved in the discussion around deps of Context. Would taking a compile time dependency on ErrorProne annotations be feasible?

One of Context users, Zipkin instrumentation libs, has no deps, not even annotation jars. So this PR is deliberately not adding any dependencies. So com.google.errorprone.annotations.CheckReturnValue can not be used in io.grpc.Context. Instead, it uses io.grpc.Context.CheckReturnValue. It is up to the compiler, whether or not to apply the errorprone plug-in when compiling (in static analysis phase). Internally, the JavaBuilder will apply errorprone by default for all projects.

new Exception());
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This probably deserves a comment why it isn't the error prone version.

@carl-mastrangelo carl-mastrangelo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one comment, but LGTM

@dapengzhang0

Copy link
Copy Markdown
Contributor Author

Thanks for the review @carl-mastrangelo .

@dapengzhang0
dapengzhang0 merged commit bf4a00c into grpc:master May 15, 2018
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 18, 2019
@dapengzhang0
dapengzhang0 deleted the contextcheckreturn branch June 1, 2019 20:17
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.

2 participants