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

Remove two pool-membership conditions guarding the C++ equivalent of obj.SerializePartialToString() #142

Closed
wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link

@copybara-service copybara-service bot commented Dec 8, 2023

Remove two pool-membership conditions guarding the C++ equivalent of obj.SerializePartialToString()

The main change in this CL is to remove two conditions in PyProtoIsCompatible():

  if (descriptor->file()->pool() != DescriptorPool::generated_pool()) {
    return py_pool->is(GlobalState::instance()->global_pool());

Rationale for removing these conditions:

  • All that matters for protobuf compatibility is that the full_name is the same. (Thanks @kmoffett for that insight!)

  • Cross-extension-module ABI compatibility is not a concern because only the Python API is used in the relevant code paths serializing Python protobuf objects to Python bytes (equivalent to calling obj.SerializePartialToString() from Python).

All other changes in this CL are secondary: small-scale refactoring, slight naming changes, additional tests for error conditions.

@copybara-service copybara-service bot changed the title PyProtoSerializePartialToString() The main change in this CL is to remove two conditions in PyProtoIsCompatible(): Dec 9, 2023
@copybara-service copybara-service bot changed the title The main change in this CL is to remove two conditions in PyProtoIsCompatible(): Remove two pool-membership conditions guarding the C++ equivalent of obj.SerializePartialToString() Dec 10, 2023
@copybara-service copybara-service bot closed this Dec 11, 2023
@copybara-service copybara-service bot deleted the cl/589031333 branch December 11, 2023 20:04
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.

0 participants