Skip to content

L27: C++ API review#70

Closed
vjpai wants to merge 4 commits intogrpc:masterfrom
vjpai:L27_cxx_api_review
Closed

L27: C++ API review#70
vjpai wants to merge 4 commits intogrpc:masterfrom
vjpai:L27_cxx_api_review

Conversation

@vjpai
Copy link
Copy Markdown
Contributor

@vjpai vjpai commented Mar 14, 2018

Continues the internalization effort started in #28 . Implemented in grpc/grpc#14648 .

Discussion thread at https://groups.google.com/forum/#!topic/grpc-io/LTFUwLhqEHE

Seeking feedback from @makdharma , @chwarr , and any others that care to discuss.

Earliest possible merger is March 30, 2018.

internal/advanced/specialized use as appropriate if typical
applications are not expected to use them. These are particularly
relevant for convenience libraries that wrap gRPC features but
provide their own tuning parameters.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think there is no need to distinguish the last two categories for users as both are implementation details inside of C++ layer.

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.

@yang-g, if the last two categories go away, how does someone implementing code generation know what APIs are available at that layer? gRFC #28 and its implementation moved some things into an "internal" namespace, e.g., ::grpc::ClientReader<R>::internal::Create. This implies that some parts of grpc::internal are not implementation details, but part of a lower-level/more advanced API.

Copy link
Copy Markdown

@yang-g yang-g left a comment

Choose a reason for hiding this comment

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

LGTM


[Previous work](https://github.com/grpc/proposal/pull/28) started to
separate internal-only from public sections of the gRPC C++ API by
moving selected compoennts to namespace `grpc::internal`. That effort
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.

s/compoennts/components/

@dfawley
Copy link
Copy Markdown
Member

dfawley commented Oct 24, 2018

For consistency, please rename this file to L27-cpp-... before merging. Thanks!

@vjpai
Copy link
Copy Markdown
Contributor Author

vjpai commented May 28, 2021

Abandoned.

@vjpai vjpai closed this May 28, 2021
@vjpai vjpai deleted the L27_cxx_api_review branch May 28, 2021 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants