Improve C# reflection support #566
Conversation
- Added new line at the end of SampleEnum - Moved GeneratedMessageTest.GetSampleMessage to a new class, SampleMessages, and renamed it to CreateFullTestAllTypes.
|
@jtattermusch I've rebased this to include PR #577 (and now that earlier PRs are merged, this looks somewhat smaller). |
- FieldAccessorTable is now non-generic - We don't have a static field per message type in the umbrella class. (Message descriptors are accessed via the file descriptor.) - Removed the "descriptor assigner" complication from the descriptor fixup; without extensions, we don't need it - MapField implements IDictionary (more tests would be good...) - RepeatedField implements IList (more tests would be good) - Use expression trees to build accessors. (Will need to test this on various platforms... probably need a fallback strategy just using reflection directly.) - Added FieldDescriptor.IsMap - Added tests for reflection with generated messages Changes to generated code coming in next commit.
- Add a partial method called by all constructors - Generate internal classes for descriptor.proto (only) - Forbid proto2 descriptors except for descriptor.proto
Note that now we need a proto3 version of addressbook.proto. This may affect other platforms, and could do with an overhaul to follow proto3 conventions anyway (e.g. repeated field names). Will need to think about that carefully before merging into master. Raised issue protocolbuffers#565 for this.
- The protos are no longer publicly exposed at all - Oneof detection now works (as we default to -1, not 0) - OneofDescriptor exposes the fields in the oneof - Removed unnecessary code for replacing protos - remnant of extensions - There's now just the non-generic form of IDescriptor
... and some implementation changes to go with them.
| // Commented assertions show an ideal situation - it looks like | ||
| // the LinkedList enumerator doesn't throw when you ask for the current entry | ||
| // at an inappropriate time; fixing this would be more work than it's worth. | ||
| // Assert.Throws<InvalidOperationException>(() => enumerator.Current.GetHashCode()); |
There was a problem hiding this comment.
Just curious, why would fixing it bee more work than it's worth?
There was a problem hiding this comment.
Because no-one actually relies on this (C# iterator blocks don't do the right thing either), and fixing it in our code (bearing in mind that we can't change LinkedList) would mean keeping a separate field to remember whether or not we're on the first/last element, etc.
|
So far, I've reviewed the changes around descriptors (which LGTM). I will continue reviewing the reflection part tomorrow. |
|
Thanks. I'll try to make a few more changes to the JSON formatting so that I can add that PR to be reviewed. Then there's the wrapper type support, which isn't ready yet... |
| @@ -68,12 +68,12 @@ public static FieldCodec<int> ForSInt32(uint tag) | |||
|
|
|||
| public static FieldCodec<uint> ForFixed32(uint tag) | |||
| { | |||
| return new FieldCodec<uint>(input => input.ReadFixed32(), (output, value) => output.WriteFixed32(value), CodedOutputStream.ComputeFixed32Size, tag); | |||
| return new FieldCodec<uint>(input => input.ReadFixed32(), (output, value) => output.WriteFixed32(value), 4, tag); | |||
There was a problem hiding this comment.
I consider the magic numbers less readable and more error prone that the previous version that clearly indicated what is the purpose of the number.
|
The reflection changes also LGTM. Feel free to merge if it speeds you up. |
|
Thanks, will do. |
…enum inner options, see protocolbuffers#565; Other minor optimizations
This is all relative to PR #561.
After this PR, reflection support is pretty reasonable. Outstanding changes I'd like to make (probably after the first merge)
@anandolee to review (sorry!)