Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145423002:
ProtoZero plugin implementation. (Closed)
DescriptionProtoZero plugin implementation.
Continuation of crrev.com/2147613002 with complete implementation of
generator plugin. Appenders are commented before getting final
version of the runtime.
Plugin output example: pastebin.com/HXyTwETy
Conventional protoc's C++ output: pastebin.com/X0vxYqgK
BUG=608721
Committed: https://crrev.com/4a7290ed1374f978aa05c068913acc6e9bb38f67
Cr-Commit-Position: refs/heads/master@{#405826}
Patch Set 1 : preprocess #Patch Set 2 : implementation #
Total comments: 40
Patch Set 3 : fixes #
Messages
Total messages: 26 (14 generated)
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kraynov@chromium.org
kraynov@chromium.org changed reviewers: + primiano@chromium.org
Looks good, I have a bunch of questions. Not sure why there are lot of things commented out. I'd like the commit message to have two urls: one that shows example_message.proto pb.h generated by cpp.h and one which shows the one generated by your plugin. It's easier to compare. In general remeember that the generated C++ stubs should be a subset but API-equivalent to the official ones. Enums here look a bit different. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:29: // are presented in //base and it's better than reinventing a wheel or mixing up NO need for this comment. In general it would be ideal to use base, as that reduces the dependency on protobuf API surface, which might change over time and make it hard to roll new versions of protobuf https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:31: using google::protobuf::Split; don't you have this in base? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:35: using google::protobuf::UpperString; You have base::ToUpperASCII https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:50: PrintPrologue(); S/Print/Generate/ here and below https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:91: StripString(&name, ".", '_'); use base https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:98: inline bool IsEnumOptimal(const EnumDescriptor* enumeration) { s/IsEnumOptimal/IsTinyEnum/ https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:112: for (const std::string& ns : namespaces_) { don't need the braces for single line loops https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:132: for (const Descriptor* message : messages_) { are youy sure this loop should not go out? Feels like you are re-iterating over the same messages over and over, while you populate the messages_ at the same time. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:146: UpperString(&guard); use base for these https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:156: "#ifndef PROTO_ZERO_NO_RUNTIME\n" what is this PROTO_ZERO_NO_RUNTIME? I think you can just uncomment the include below now https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:181: "enum $class$ : int32_t {\n", It seems that the standard proto just generates an plain enum, not enum class. Any reason why we are diverging here? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:189: "number", std::to_string(value->number())); is this going to generate enums according to the same pattern of protoc? E.g., enum OmniboxEventProto_Suggestion_ResultType { OmniboxEventProto_Suggestion_ResultType_UNKNOWN_RESULT_TYPE = 0, OmniboxEventProto_Suggestion_ResultType_URL_WHAT_YOU_TYPED = 1, OmniboxEventProto_Suggestion_ResultType_HISTORY_URL = 2, OmniboxEventProto_Suggestion_ResultType_HISTORY_TITLE = 3, OmniboxEventProto_Suggestion_ResultType_HISTORY_BODY = 4, ... } doesn't seem so at first glance https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:271: if (IsEnumOptimal(field->enum_type())) { appender = IsTinyEnum(field.enum_type()) ? "AppendTinyInt" : "AppendInt32" https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:288: " // AppendBytes($id$, data, size);\n" Why AppendBytes is commented out? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:302: " // $appender$($id$, value);\n" why the appender is commented? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:321: "class $name$ /* : public ::tracing::proto::ProtoZeroMessage */ {\n" why this is commented? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:346: Abort("Required fields are not supported."); why this? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:353: if (field->type() != FieldDescriptor::TYPE_MESSAGE) { Is the case of nested messages not handled yet?
https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:29: // are presented in //base and it's better than reinventing a wheel or mixing up On 2016/07/15 10:33:56, Primiano Tucci wrote: > NO need for this comment. In general it would be ideal to use base, as that > reduces the dependency on protobuf API surface, which might change over time and > make it hard to roll new versions of protobuf Discussed offline. Not going to depend on //base at all. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:35: using google::protobuf::UpperString; On 2016/07/15 10:33:56, Primiano Tucci wrote: > You have base::ToUpperASCII Discussed offline. Not going to change. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:50: PrintPrologue(); On 2016/07/15 10:33:56, Primiano Tucci wrote: > S/Print/Generate/ here and below Acknowledged. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:91: StripString(&name, ".", '_'); On 2016/07/15 10:33:56, Primiano Tucci wrote: > use base Discussed offline. Not going to change. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:98: inline bool IsEnumOptimal(const EnumDescriptor* enumeration) { On 2016/07/15 10:33:55, Primiano Tucci wrote: > s/IsEnumOptimal/IsTinyEnum/ Acknowledged. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:112: for (const std::string& ns : namespaces_) { On 2016/07/15 10:33:56, Primiano Tucci wrote: > don't need the braces for single line loops Acknowledged. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:132: for (const Descriptor* message : messages_) { On 2016/07/15 10:33:56, Primiano Tucci wrote: > are youy sure this loop should not go out? Feels like you are re-iterating over > the same messages over and over, while you populate the messages_ at the same > time. I iterate over messages_ and populate enums_. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:146: UpperString(&guard); On 2016/07/15 10:33:56, Primiano Tucci wrote: > use base for these Discussed offline. Not going to change. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:156: "#ifndef PROTO_ZERO_NO_RUNTIME\n" On 2016/07/15 10:33:56, Primiano Tucci wrote: > what is this PROTO_ZERO_NO_RUNTIME? > I think you can just uncomment the include below now #ifndef here. PROTO_ZERO_NO_RUNTIME is going to be defined in the unittest in order to substitute with a fake one. See: https://codereview.chromium.org/2083373002/patch/60001/70012 fake_runtime.h here defines PROTO_ZERO_NO_RUNTIME. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:189: "number", std::to_string(value->number())); On 2016/07/15 10:33:56, Primiano Tucci wrote: > is this going to generate enums according to the same pattern of protoc? > E.g., > > enum OmniboxEventProto_Suggestion_ResultType { > OmniboxEventProto_Suggestion_ResultType_UNKNOWN_RESULT_TYPE = 0, > OmniboxEventProto_Suggestion_ResultType_URL_WHAT_YOU_TYPED = 1, > OmniboxEventProto_Suggestion_ResultType_HISTORY_URL = 2, > OmniboxEventProto_Suggestion_ResultType_HISTORY_TITLE = 3, > OmniboxEventProto_Suggestion_ResultType_HISTORY_BODY = 4, > ... > } > > doesn't seem so at first glance Ok, thanks for pointing on that! https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:271: if (IsEnumOptimal(field->enum_type())) { On 2016/07/15 10:33:56, Primiano Tucci wrote: > appender = IsTinyEnum(field.enum_type()) ? "AppendTinyInt" : "AppendInt32" Acknowledged. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:288: " // AppendBytes($id$, data, size);\n" On 2016/07/15 10:33:56, Primiano Tucci wrote: > Why AppendBytes is commented out? It's to be uncommentend in the next CL when API will be finally negotiated. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:302: " // $appender$($id$, value);\n" On 2016/07/15 10:33:56, Primiano Tucci wrote: > why the appender is commented? It's to be uncommentend in the next CL when API will be finally negotiated. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:346: Abort("Required fields are not supported."); On 2016/07/15 10:33:56, Primiano Tucci wrote: > why this? required considered harmful and we aren't going to enforce this constraint on generation stage. Hence parser could get invalid binary. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:353: if (field->type() != FieldDescriptor::TYPE_MESSAGE) { On 2016/07/15 10:33:56, Primiano Tucci wrote: > Is the case of nested messages not handled yet? It's handled in PrintNestedMessageFieldDescriptor.
https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:132: for (const Descriptor* message : messages_) { On 2016/07/15 11:01:53, kraynov wrote: > On 2016/07/15 10:33:56, Primiano Tucci wrote: > > are youy sure this loop should not go out? Feels like you are re-iterating > over > > the same messages over and over, while you populate the messages_ at the same > > time. > > I iterate over messages_ and populate enums_. Oh ignore my comment, I didn't realize that the while(!stack.empty()) brace was closed above. I'add some blank lines between the loops to make the code a bit more redable. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:156: "#ifndef PROTO_ZERO_NO_RUNTIME\n" On 2016/07/15 11:01:52, kraynov wrote: > On 2016/07/15 10:33:56, Primiano Tucci wrote: > > what is this PROTO_ZERO_NO_RUNTIME? > > I think you can just uncomment the include below now > > #ifndef here. PROTO_ZERO_NO_RUNTIME is going to be defined in the unittest in > order to substitute with a fake one. See: > https://codereview.chromium.org/2083373002/patch/60001/70012 fake_runtime.h here > defines PROTO_ZERO_NO_RUNTIME. Hmm feels very weird that all emitted messages have this ifdef just because in one case you want to mock it. ALso this means that if I want to make a change to the ProtoZeroMessage, now I have to match the API change in the fake runtime. I wonder if the best testing strategy here is just using the official runtime. What's the benefit of the fake one? https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:310: " // OpenNestedMessage($id$, &nested);\n" This is BeginNestedMessage. See https://cs.chromium.org/chromium/src/components/tracing/core/proto_zero_messa... https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:346: Abort("Required fields are not supported."); On 2016/07/15 11:01:53, kraynov wrote: > On 2016/07/15 10:33:56, Primiano Tucci wrote: > > why this? > > required considered harmful and we aren't going to enforce this constraint on > generation stage. Hence parser could get invalid binary. I know but doesn't seem an argument for not emitting code for that. I'd postpone this decision to codereviews of the actual protos. Isn't going to make a huge difference here. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:353: if (field->type() != FieldDescriptor::TYPE_MESSAGE) { On 2016/07/15 11:01:52, kraynov wrote: > On 2016/07/15 10:33:56, Primiano Tucci wrote: > > Is the case of nested messages not handled yet? > > It's handled in PrintNestedMessageFieldDescriptor. Acknowledged.
https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:156: "#ifndef PROTO_ZERO_NO_RUNTIME\n" On 2016/07/15 11:53:12, Primiano Tucci wrote: > On 2016/07/15 11:01:52, kraynov wrote: > > On 2016/07/15 10:33:56, Primiano Tucci wrote: > > > what is this PROTO_ZERO_NO_RUNTIME? > > > I think you can just uncomment the include below now > > > > #ifndef here. PROTO_ZERO_NO_RUNTIME is going to be defined in the unittest in > > order to substitute with a fake one. See: > > https://codereview.chromium.org/2083373002/patch/60001/70012 fake_runtime.h > here > > defines PROTO_ZERO_NO_RUNTIME. > Hmm feels very weird that all emitted messages have this ifdef just because in > one case you want to mock it. > ALso this means that if I want to make a change to the ProtoZeroMessage, now I > have to match the API change in the fake runtime. > > I wonder if the best testing strategy here is just using the official runtime. > What's the benefit of the fake one? Ok, will remove for this time. https://codereview.chromium.org/2145423002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:346: Abort("Required fields are not supported."); On 2016/07/15 11:53:12, Primiano Tucci wrote: > On 2016/07/15 11:01:53, kraynov wrote: > > On 2016/07/15 10:33:56, Primiano Tucci wrote: > > > why this? > > > > required considered harmful and we aren't going to enforce this constraint on > > generation stage. Hence parser could get invalid binary. > > I know but doesn't seem an argument for not emitting code for that. I'd postpone > this decision to codereviews of the actual protos. > Isn't going to make a huge difference here. Acknowledged.
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
lgtm
alph@chromium.org changed reviewers: + alph@chromium.org
A dumb question. Why the generator is not in Python?
On 2016/07/15 17:45:06, alph wrote: > A dumb question. Why the generator is not in Python? protoc (official protobuf compiler) is written in C++ and plugin support is essentially an approach to add another one generator from standalone executable.
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/15 17:45:06, alph wrote: > A dumb question. Why the generator is not in Python? That was my original attempt, which was working as you can see in https://codereview.chromium.org/1947373002/diff/260001/base/trace_event/v2/tr... but was really a hacky approach. The problems were the following: 1. Definitely I don't want to write my own .proto parser, that seems a waste of time and resources, and also maintenance pain. 2. There is no easy way AFAIK to access the proto parser API from python. What I did there was the following: use protoc to generate the .py stub classes, and then use introspection in python to just reconstruct the classes taxonomy. It was all a bit wacky. 3. The other big problem, a bit silly but still a bummer, is that there is no easy way to import the python protobuf code from the tree without requiring people to install it. You can follow a longer conversation on https://github.com/google/protobuf/issues/1484 . I am not a python package expert but folks there are convinced that is WAI the fact that you cannot just import python version protobuf from a folder. Which was a bit of a blocker. I managed to work around that, but that was adding another layer of packaging black magic workarounds. At the end I started an thread asking advice to the internal protobuf mailing list and they deemed the python solution as "quite creative" and strongly suggested to move to the plugin system. I just ended up trusting their advice (internal thread: go/gdhub) Effectively this feels a more robust and stable solution than the python hack.
Description was changed from ========== ProtoZero plugin implementation. Continuation of crrev.com/2147613002 with complete implementation of generator plugin. At this time it does not depend on actual runtime and all the invocations will be commented in generated code stubs. BUG=608721 ========== to ========== ProtoZero plugin implementation. Continuation of crrev.com/2147613002 with complete implementation of generator plugin. Appenders are commented before getting final version of the runtime. Plugin output example: pastebin.com/HXyTwETy Conventional protoc's C++ output: pastebin.com/X0vxYqgK BUG=608721 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kraynov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ProtoZero plugin implementation. Continuation of crrev.com/2147613002 with complete implementation of generator plugin. Appenders are commented before getting final version of the runtime. Plugin output example: pastebin.com/HXyTwETy Conventional protoc's C++ output: pastebin.com/X0vxYqgK BUG=608721 ========== to ========== ProtoZero plugin implementation. Continuation of crrev.com/2147613002 with complete implementation of generator plugin. Appenders are commented before getting final version of the runtime. Plugin output example: pastebin.com/HXyTwETy Conventional protoc's C++ output: pastebin.com/X0vxYqgK BUG=608721 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== ProtoZero plugin implementation. Continuation of crrev.com/2147613002 with complete implementation of generator plugin. Appenders are commented before getting final version of the runtime. Plugin output example: pastebin.com/HXyTwETy Conventional protoc's C++ output: pastebin.com/X0vxYqgK BUG=608721 ========== to ========== ProtoZero plugin implementation. Continuation of crrev.com/2147613002 with complete implementation of generator plugin. Appenders are commented before getting final version of the runtime. Plugin output example: pastebin.com/HXyTwETy Conventional protoc's C++ output: pastebin.com/X0vxYqgK BUG=608721 Committed: https://crrev.com/4a7290ed1374f978aa05c068913acc6e9bb38f67 Cr-Commit-Position: refs/heads/master@{#405826} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4a7290ed1374f978aa05c068913acc6e9bb38f67 Cr-Commit-Position: refs/heads/master@{#405826} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
