Support oneofs in MRI Ruby C extension. #168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@haberman to review. |
- A golden-file test that ensures protoc produces known-valid output. - A Ruby test that loads that golden file and ensures it actually works with the extension. This split strategy allows us to test end-to-end without needing to integrate the Ruby gem build system and the protoc build system. This is desirable because we do not want a gem build/install to depend on building protoc, and we do not want building protoc to depend on building and testing the gem.
|
Updated Ruby code generator as well. |
|
|
||
| /* | ||
| * call-seq: | ||
| * Descriptor.oneofs => list of OneofDescriptors |
There was a problem hiding this comment.
More idiomatic to define:
Descriptor.each_oneof { |oneof| }
?
|
Thanks! PTAL. |
| @@ -65,6 +65,10 @@ static upb_fielddef* check_field_notfrozen(const upb_fielddef* def) { | |||
| return (upb_fielddef*)check_notfrozen((const upb_def*)def); | |||
There was a problem hiding this comment.
upb_downcast_fielddef() / UPB_UPCAST(). Should be safer than doing casts directly.
| desc->layout->offsets[upb_fielddef_index(f)] + sizeof(MessageHeader); | ||
| desc->layout->fields[upb_fielddef_index(f)].offset + | ||
| sizeof(MessageHeader); | ||
| uint32_t oneof_case_offset = |
| rb_class_new_instance(0, NULL, subklass); | ||
| } | ||
| // Set the oneof case *after* allocating the new class instance -- see comment | ||
| // in layout_set() as to why. There are subtle interactions with possible GC |
There was a problem hiding this comment.
I would specifically mention here that the key invariant is that setting of data and case are atomic.
|
Thanks! PTAL. |
| uint32_t oneof_case = *((uint32_t*)(Message_data(self) + case_ofs)); | ||
|
|
||
| // oneof_case == 0 indicates no field set. | ||
| if (oneof_case == 0) { |
There was a problem hiding this comment.
Use ONEOF_CASE_NONE and rm comment.
There was a problem hiding this comment.
Missed that one, sorry! Grepped for oneof_case and I don't think there are any more literal zeroes. Done.
haberman
added a commit
that referenced
this pull request
Feb 2, 2015
Support oneofs in MRI Ruby C extension.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for oneofs -- in the defs, in message objects, and during parsing and serialization -- to the Ruby extension. The API impact is minimal: fields that are part of a oneof can now take on a
nilvalue to indicate not-present, and at most one field in a oneof may be present (non-nil).This includes a (necessary) update of the upb amalgamation up to git commit
1988a660.