Skip to content

Move create_channel_posix from grpc to grpc_impl namespace#18373

Merged
karthikravis merged 8 commits intomasterfrom
grpc_namespace_channel_create_posix
Mar 29, 2019
Merged

Move create_channel_posix from grpc to grpc_impl namespace#18373
karthikravis merged 8 commits intomasterfrom
grpc_namespace_channel_create_posix

Conversation

@karthikravis
Copy link
Copy Markdown
Contributor

No description provided.

@karthikravis karthikravis added the release notes: no Indicates if PR should not be in release notes label Mar 14, 2019
@karthikravis karthikravis self-assigned this Mar 14, 2019
@karthikravis karthikravis force-pushed the grpc_namespace_channel_create_posix branch from 30409e2 to 4b0175f Compare March 25, 2019 22:54

namespace grpc {

#ifdef GPR_SUPPORT_CHANNELS_FROM_FD
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.

I never realized we had that. Gosh this is ugly. We should make a note to clean this up later.

Rule of thumb: no public API should be behind an ifdef like this. The actual implementation might differ depending on said define, and assert with a message saying this feature isn't enabled in this version, but the API/ABI shouldn't mutate depending on a compilation flag.

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.

CC @vjpai

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Filed #18571 to track.

@karthikravis karthikravis merged commit 3354f2c into master Mar 29, 2019
@karthikravis karthikravis deleted the grpc_namespace_channel_create_posix branch May 6, 2019 23:51
@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants