Skip to content

Remove g_glip and class GrpcLibraryInterface#30414

Merged
ralphchung merged 35 commits intogrpc:masterfrom
ralphchung:eliminate-impl-codegen-directories
Nov 14, 2022
Merged

Remove g_glip and class GrpcLibraryInterface#30414
ralphchung merged 35 commits intogrpc:masterfrom
ralphchung:eliminate-impl-codegen-directories

Conversation

@ralphchung
Copy link
Copy Markdown
Contributor

@ralphchung ralphchung commented Jul 26, 2022

This PR requires cherry picking before merging.

This PR aims to remove the pointer g_glip and class GrpcLibraryInterface.

Details:

  • grpc::GrpcLibraryCodegen is moved to grpc::GrpcLibrary to replace the need for pointer g_glip.
  • grpc::GrpcLibraryInterface is removed.

Copy link
Copy Markdown
Contributor

@Vignesh2208 Vignesh2208 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me.


private:
bool grpc_init_called_;
static internal::GrpcLibrary internal_grpc_library;
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.

Why is there a static grpc::internal::GrpcLibrary ? It seems like its methods only call grpc_init() and grpc_shutdown(). Why not call them directly in grpc::GrpcLibrary constructor and destructor respectively ?

/*
*
* Copyright 2018 gRPC authors.
* Copyright 2022 gRPC authors.
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.

Please switch to newer style C++ header/copyright comments in this the file (e.g, src/core/lib/event_engine/iomgr_engine/ev_epoll1_linux.cc)

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.

Done

@ctiller
Copy link
Copy Markdown
Member

ctiller commented Jul 27, 2022

Just noting here: this will need an internal cherry-pick, as there are internal usages of this header.

@ralphchung ralphchung force-pushed the eliminate-impl-codegen-directories branch from ed520c6 to d52be99 Compare October 18, 2022 17:06
@ralphchung ralphchung force-pushed the eliminate-impl-codegen-directories branch from d52be99 to 989d5fd Compare October 27, 2022 03:16
@ralphchung ralphchung force-pushed the eliminate-impl-codegen-directories branch from 989d5fd to 2f5c77e Compare October 27, 2022 03:47
This reverts commit 1ee3ae4.
This reverts commit 1fc2348.
This reverts commit 851393c.
This reverts commit ed342a5.
This reverts commit 286ad8e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bloat/none disposition/Needs Internal Changes imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral 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.

5 participants