Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2293073002:
Tracing V2: Proto fields reflection. (Closed)
DescriptionTracing V2: Proto fields reflection.
Expose field name, type, number and is it repeated or not.
BUG=608721
Committed: https://crrev.com/046fd5415e76b39b550ae7b246120d1801ac4cef
Cr-Commit-Position: refs/heads/master@{#416464}
Patch Set 1 #
Total comments: 14
Patch Set 2 : new one #
Total comments: 14
Patch Set 3 : simplier #Patch Set 4 : try to fix tests #Patch Set 5 : attempt 2 #Patch Set 6 : no assumption #Patch Set 7 : nit #
Total comments: 3
Patch Set 8 : nit #
Messages
Total messages: 46 (33 generated)
kraynov@chromium.org changed reviewers: + primiano@chromium.org
PTAL
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:48: struct FieldReflection { Can this have the same shape of https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot... which means - make this a class FieldDescriptor (not struct) - make the num above an inner public enum of this class and call it "Type" - use the same name and values of that one, e.g.: TYPE_DOUBLE = 1, TYPE_FLOAT = 2, etc. You just need the one that we support. - add a Type type() const {...} getter - add a uint32_t number() getter; - add a const char* name() getter (no need to use std::string) - ditto for is_repeated() https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:65: const std::string name; const char* const https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:69: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:104: inline std::string Capitalize(const std::string& str) { honestly I feel this is just overkill as you use only in one place. Just easier if online 111 you do: std::string name = field->camelcase_name(); name[0] = std::toupper(name[0]) name = "k" + name + "FieldNumer" (there should be no need for the explicit std::string("k")) https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:248: "#include <stdint.h>\n\n" I think you need also cctype for std::toupper https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:457: // Field numbers. I'd make this more clear: // Field Numbers (e.g., kXXXFieldNumber = 42) https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:459: stub_h_->Print("enum : int32_t {\n"); just realized this, so far we used uint32_t for field numbers. https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:489: " return ::tracing::v2::proto::FieldReflection(\"$name$\", " Hmm this is going to be slow. Can we make this just a lookup in a table where you have all the descriptors, so this returns a FieldDescirptor*. Something along the lines of http://pastebin.com/WPJWsnJH . I think that if the table is fully const that should not require any static initializers, as all the data will end up in the .rodata (or equivalent). So this can generate just something like: switch(field_id) { case 1: return &DescriptorTable[12]; case 2: return &DescriptorTable[7]; etc }
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...
Will fix nits soon https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:48: struct FieldReflection { On 2016/08/31 09:04:07, Primiano Tucci wrote: > Can this have the same shape of > https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot... > which means > > - make this a class FieldDescriptor (not struct) > - make the num above an inner public enum of this class and call it "Type" > - use the same name and values of that one, e.g.: > TYPE_DOUBLE = 1, > TYPE_FLOAT = 2, > etc. You just need the one that we support. > > - add a Type type() const {...} getter > - add a uint32_t number() getter; > - add a const char* name() getter (no need to use std::string) > - ditto for is_repeated() Done. https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:65: const std::string name; On 2016/08/31 09:04:07, Primiano Tucci wrote: > const char* const Done. https://codereview.chromium.org/2293073002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:69: }; On 2016/08/31 09:04:07, Primiano Tucci wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:457: // Field numbers. On 2016/08/31 09:04:07, Primiano Tucci wrote: > I'd make this more clear: > // Field Numbers (e.g., kXXXFieldNumber = 42) Acknowledged. https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:459: stub_h_->Print("enum : int32_t {\n"); On 2016/08/31 09:04:07, Primiano Tucci wrote: > just realized this, so far we used uint32_t for field numbers. Acknowledged. https://codereview.chromium.org/2293073002/diff/1/components/tracing/tools/pr... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:489: " return ::tracing::v2::proto::FieldReflection(\"$name$\", " On 2016/08/31 09:04:07, Primiano Tucci wrote: > Hmm this is going to be slow. > Can we make this just a lookup in a table where you have all the descriptors, so > this returns a FieldDescirptor*. > Something along the lines of http://pastebin.com/WPJWsnJH . > I think that if the table is fully const that should not require any static > initializers, as all the data will end up in the .rodata (or equivalent). > So this can generate just something like: > > switch(field_id) { > case 1: > return &DescriptorTable[12]; > case 2: > return &DescriptorTable[7]; > etc > > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
LGTM with some final comments, please look at them. THe major comment is avoid unnecessary switches (reflection=true/false) unless really needed. That reduces code mainteinance. Like the overall approach, quite smart :) ALso please make the commit message a bit more descriptive. Remember that the main question a commit message should answer is "why do we need this?" and should be explicative enough so that a random chrome developer can make a sense out of it. https://codereview.chromium.org/2293073002/diff/20001/components/tracing/BUIL... File components/tracing/BUILD.gn (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/BUIL... components/tracing/BUILD.gn:79: generator_plugin_options = "wrapper_namespace=pbzero,reflection=true" i'd avoid unnecessary switches. we just need this. We can add a switch if we end up not needing it. In general Chrome philosophy is to never overdesign: more codepaths == less readable code == harder maintenance https://codereview.chromium.org/2293073002/diff/20001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:51: ProtoFieldDescriptor(const char* name, is this git-cl formatted? https://codereview.chromium.org/2293073002/diff/20001/components/tracing/prot... File components/tracing/proto/BUILD.gn (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/prot... components/tracing/proto/BUILD.gn:20: generator_plugin_options = "wrapper_namespace=pbzero,reflection=true" ditto https://codereview.chromium.org/2293073002/diff/20001/components/tracing/test... File components/tracing/test/proto_zero_generation_unittest.cc (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/test... components/tracing/test/proto_zero_generation_unittest.cc:187: #define COMPARE_FIELD_TYPE_VALUES(value) \ nit: don't indent defines https://codereview.chromium.org/2293073002/diff/20001/components/tracing/test... components/tracing/test/proto_zero_generation_unittest.cc:210: #undef COMPARE_FIELD_TYPE_VALUES ditto, remove trailing spaces. https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:65: } else if (name == "reflection") { ditto, less code to maintain plz :) https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:294: "__invalid_field = {\"\", " just calling kInvalidField should be enough and fits our naming conventions. https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:497: "__fields_$class$[] = {\n", ditto, just kFields... as this is a non exported symbol is extremely unlikely to clash, no need to __ magic I think :) https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:504: // same numbers for corresponding types. Hence, there is a test. s/Hence, there is a test./This is covered by TEST_NAME in ...blabla_unittest.cc/
picksi@chromium.org changed reviewers: + picksi@chromium.org
Some random thoughts from me :) I clearly have too much time on my hands... Feel free to ignore. https://codereview.chromium.org/2293073002/diff/20001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:48: // TYPE_GROUP = 10 is not supported. Should this comment be put at the 'correct' place in the enum (i.e. between 9 and 11)? Is there a danger a future developer will see the gap, not read the comment, and re-use 10 for something else? https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:67: generate_reflection_ = true; There is an implication that generate_reflection defaults to false here, which may change at some future point? Should this if/else (or switch if it changes) set generate_reflection to false if value == 'false' || value = '1' and move this Abort into the else branch. https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:111: inline std::string Capitalize(const std::string& str) { Could you use Capitalize() in the SetOption function and compare with TRUE and FALSE, as currently passing 'True' or 'False' (i.e. capitalized) would fail. https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:473: if (message->field_count() > 0) { There are a number of places where the code has this if >0 statement. If we own the Descriptor class (not sure?) should we add a new function called has_fields() to wrap this repeated functionality? https://codereview.chromium.org/2293073002/diff/20001/components/tracing/tool... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:554: GenerateReflectionHelpers(message); Is this function correctly named GenerateReflectionHelpers()? The code reads slightly oddly as the condition is called generater_reflection, so should this function just be called GenerateReflection()?
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 commit-bot@chromium.org
Dry run: Exceeded global retry quota
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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 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...
Still LGTM,just some final small comments. https://codereview.chromium.org/2293073002/diff/120001/components/tracing/too... File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right): https://codereview.chromium.org/2293073002/diff/120001/components/tracing/too... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:107: name = std::string("k") + name + "FieldNumber"; you can avoid std::string and just "k" here. https://codereview.chromium.org/2293073002/diff/120001/components/tracing/too... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:459: // TODO(kraynov): Implement reflection for enums. Not sure we need this. I'd remove this todo. If we need it I'll tell you :) https://codereview.chromium.org/2293073002/diff/120001/components/tracing/too... components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:494: type_const += FieldDescriptor::TypeName(field->type()); maybe you can do this in one line and to: type_const += UpperString(FieldDescriptor::TypeName(field->type())) if it fits in one line
The CQ bit was checked by kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2293073002/#ps130001 (title: "nit")
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 commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Tracing V2: Proto fields reflection. Expose field name, type, number and is it repeated or not. BUG=608721 ========== to ========== Tracing V2: Proto fields reflection. Expose field name, type, number and is it repeated or not. BUG=608721 Committed: https://crrev.com/046fd5415e76b39b550ae7b246120d1801ac4cef Cr-Commit-Position: refs/heads/master@{#416464} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/046fd5415e76b39b550ae7b246120d1801ac4cef Cr-Commit-Position: refs/heads/master@{#416464} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
