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

Implemented proto3 presence for Ruby. #7406

Merged
merged 5 commits into from Apr 23, 2020

Conversation

haberman
Copy link
Member

@haberman haberman commented Apr 21, 2020

No description provided.

@haberman haberman requested a review from TeBoring Apr 21, 2020
@TeBoring TeBoring added release notes: yes ruby kokoro:run labels Apr 21, 2020
continue;
}

// Prepend '_' until we are no longer conflicting.
Copy link
Contributor

@TeBoring TeBoring Apr 22, 2020

Choose a reason for hiding this comment

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

Will this renaming suddenly change when users reorder fields in their proto definition?

Copy link
Member Author

@haberman haberman Apr 22, 2020

Choose a reason for hiding this comment

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

If there are conflicts it could. But the synthetic oneof name was not created by the user, so the user cannot assume what its name is.

if (accessor_type == METHOD_PRESENCE && test_f != NULL) {
if (!upb_fielddef_haspresence(test_f)) return METHOD_UNKNOWN;

// TODO(haberman): remove this case, allow for proto3 oneofs.
Copy link
Contributor

@TeBoring TeBoring Apr 22, 2020

Choose a reason for hiding this comment

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

Do we previously support this?

Copy link
Member Author

@haberman haberman Apr 22, 2020

Choose a reason for hiding this comment

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

No, we have never supported this. The unit tests were failing until I added this check, see:

assert_raise NoMethodError do
m.has_a?
end
assert_raise ArgumentError do
OneofMessage.descriptor.lookup('a').has?(m)
end

size_t hasbit = layout->fields[upb_fielddef_index(field)].hasbit;
assert(field_contains_hasbit(layout, field));
Copy link
Contributor

@TeBoring TeBoring Apr 22, 2020

Choose a reason for hiding this comment

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

Do you need to move this assert?

Copy link
Member Author

@haberman haberman Apr 22, 2020

Choose a reason for hiding this comment

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

Yes, because it is a statement and cannot go before the declaration of hasbit.

This worked previously because we had not been testing with asserts enabled.

Copy link
Contributor

@TeBoring TeBoring left a comment

What is synthetic oneof?

@haberman
Copy link
Member Author

@haberman haberman commented Apr 22, 2020

Re: synthetic oneof (and for more background information in general), see this doc which is currently in review: https://github.com/protocolbuffers/protobuf/blob/9ae5203712eb80f89261d6df8d5674efa5a0edb8/docs/implementing_proto3_presence.md

@TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Apr 22, 2020

Did ruby 2.6 fail on mac?

@haberman
Copy link
Member Author

@haberman haberman commented Apr 23, 2020

@TeBoring fixed the crash that was happening on mac.

@haberman haberman merged commit 6b75968 into protocolbuffers:master Apr 23, 2020
48 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes release notes: yes ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants