Low-level support for custom options in C##2317
Low-level support for custom options in C##2317jskeet merged 1 commit intoprotocolbuffers:masterfrom
Conversation
jskeet
force-pushed
the
custom-options
branch
from
7e4ca7e to
7e91fce
Compare
jskeet
force-pushed
the
custom-options
branch
3 times, most recently
from
a7dadd8 to
02165d5
Compare
jskeet
force-pushed
the
custom-options
branch
from
02165d5 to
e151c8b
Compare
There was a problem hiding this comment.
I think its better to decide which messages need to store unknown fields in protoc. It's a waste of code for descriptors other than options. Though only for descriptor.proto file, but worth to do so to make the code more clear.
There was a problem hiding this comment.
That would make sense. Will take a while to rework this - am at a conference at the moment. Will look next week.
There was a problem hiding this comment.
How about remove this file and put them in code gen?
There was a problem hiding this comment.
Yes, exactly - that's what I'll do next week. I expect the C# code in the support library to stay the same though, so it's still worth reviewing that.
There was a problem hiding this comment.
Other languages use templates, so users can get the options by GetExtension() instead of TryGet*. Is it possible to use template too? We promised to users the API of protobuf will be stable after 3.0.0, so we should think the APIs carefully.
There was a problem hiding this comment.
If you mean using generics, I wouldn't want to here... there are really only certain types that are feasible. We have generics for the message type, but I think it makes sense to stick to specific types for other things - we don't want to allow TryGet<Guid> for example.
For singular options, I think this is fine - and we may write code-gen to generate extra code per option anyway, which would "know" about both the type and the field number. But that can be added without changing this API at all (and indeed I'd expect it to use this).
There was a problem hiding this comment.
Yes I mean generics, I know there's some difference between C# generics and C++ template so make sense to me if you think it is better to stick to specific types.
Generate extra code per option to know the type and field number sounds good, so users can use field name instead of field number.
There was a problem hiding this comment.
Right - but that can come later. This much unblocks users immediately, giving us more time to get the codegen right. We should think about repeated options, but I think that can come a bit later too.
There was a problem hiding this comment.
Sounds good!
Does c++ support repeated options? Should be supported in c++ because it is an extension, but I didn't see unittest for that.
There was a problem hiding this comment.
I don't see any tests for that either, but I protoc allows it IIRC. The lack of any examples is one reason I don't think it's too crucial to do right now :) (In particular, I don't think it works for messages, because if an option is specified multiple times that basically means "Merge each occurrence into a single message.")
There was a problem hiding this comment.
Have replied to comments, but not pushed the new version yet as I'm still tracking down a bug. I may well add tests for the min/max values as well...
jskeet
force-pushed
the
custom-options
branch
from
e151c8b to
0faeb6c
Compare
|
Rewritten to generate the appropriate code just for options, rather than using partial methods. The PTAL @anandolee |
anandolee
left a comment
anandolee
left a comment
There was a problem hiding this comment.
LGTM in general. Thanks for the change
There was a problem hiding this comment.
Done. (Tabs are annoying...)
There was a problem hiding this comment.
Add more tests which has defined in the proto file? Like TestMessageWithCustomOptions etc.
There was a problem hiding this comment.
Added a test for every possible option (other than oneof) and a couple for aggregate options.
I'm sure more tests could be added, but I don't think they'd test anything extra at this stage.
Adding the tests has found a bug though - I haven't yet fixed it; looking into it... (The value -9876543210 is being read as 19753086419 and I don't know why.)
There was a problem hiding this comment.
Sounds good!
Does c++ support repeated options? Should be supported in c++ because it is an extension, but I didn't see unittest for that.
|
Looks like the problem is with zigzag encoding, which I'm not currently handling properly. Will need to change the CustomOptions class to mirror CodedInputStream, with all the different protobuf types (SInt32 etc). Will probably do that Friday. Definitely shows the benefits of testing - thanks! |
|
Okay, pushed a second commit with requested changes. I've left it as a second commit at the moment rather than amending the first so that the change is obvious - I'll squash before merging. Good job you requested more tests, so I was able to find the bug which suggested the API change. We dodged a bullet there :) This has also given me thoughts about what the generated options should look like when we get there - I can share that in a "not for submission" PR later... PTAL. |
|
LGTM |
This consists of: - Changing the codegen for the fixed set of options protos, to parse unknown fields instead of skipping them - Add a new CustomOptions type in the C# support library - Expose CustomOptions properties from the immutable proto wrappers in the support library Only single-value options are currently supported, and fetching options values requires getting the type right and knowing the field number. Both of these can be addressed at a later time. Fixes protocolbuffers#2143, at least as a first pass.
jskeet
force-pushed
the
custom-options
branch
from
2e8e607 to
10dd15c
Compare
|
Thanks. Found a rogue old file in Makefile.am - have pushed a new single commit for the whole PR; will merge on green. |
4 participants
Not ready for submission yet - more tests required.
Creating as PR for feedback before going any further.
Will eventually resolve #2143.
// cc @gabm.