proto_zero_plugin [NOT FOR REVIEW].

proto_zero_plugin [NOT FOR REVIEW].

Temporary CL. Sample and canary will be removed from
reviewable CL.

BUG=

[kraynov, 2016-06-22 14:11:10]

Description was changed from

==========
proto_zero_plugin [NOT FOR REVIEW].

Temporary CL. Sample and canary will be removed from
reviewable CL.

BUG=
==========

to

==========
proto_zero_plugin [NOT FOR REVIEW].

Temporary CL. Sample and canary will be removed from
reviewable CL.

BUG=
==========

[kraynov, 2016-06-22 14:11:11]

kraynov@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-07-05 11:30:18]

Had a first pass. Looks in a very good shape.
I think you have pieces from the other build-file CL you want to remove here
(rebase this on top of the other)
Some minor comments here. As a general suggestion I'd link against base and use
base methods here.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
File components/tracing/proto_zero_plugin/BUILD.gn (right):

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/BUILD.gn:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
Move all the plugin thing under a "tools" folder (components/tracing/tools).
The kinda-of pattern we have in chrome is that pieces of code which are not part
of the chrome binary are into some "tools" folder. This is to make clear that it
is not directly chrome code.
Maybe components/tracing/tools/proto_zero_plugin

Also add a README.chromium  with few lines explaining what this plugin is about.
Imagine that you are a chromium committer stumbling across this folder, you want
to give them few lines to explain: what plugin for what it is. What is the
reason of its existance.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
File components/tracing/proto_zero_plugin/proto_zero_generator.cc (right):

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:10: #include
<google/protobuf/descriptor.h>
make these absolute includes

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:22: //
TODO(kraynov) Ship "lite" runtime in order to use outside of tracing.
Not sure what this TODO means.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:23: const string
OUT_INCLUDES =
This generates a static initializer (Bad thing).
Also the name of constatns should be kCamelCase.
Make this:
const char kIncludes[] = "....";

Not sure though you need this constant at all. This seems to be used only once.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:34: void Run() {
Hmm Run makes it feel this is like part of some threading / worker pool impl,
which is not the case.
Just call this: Generate() ? or GenerateCode()?

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:39:
PrintBoilerplate();
I'd s/Print/Generate/ in these functions.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:47: // Split string
by dots.
Looks like this is reinventing base/strings/string_tokenizer.h. Just reuse that
(linking against base shouln't be a problem)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:48: vector<string>
StringTokens(const string& str) {
std::string. I think we disallow "using namespace std" or using std::string. To
make it clear which string is which.
I understand that you are not doing any direct "using" here, some of the headers
is doing that for you. But for consistency with the chrome codebase plz use
std::string. here and elsewhere.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:64: for (const
string ns : namespaces_) {
missing & (const string&), this one is unnecessarily copying the string at each
iteration (in general it is very unlikely that you want something to be const X
without &)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:65:
absolute_namespace_ += ns + "::";
this will make the abs_namespace end with "::". Do you intend to do that? Maybe
this should be the other way round (abs_namespace + = "::" + ns)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:71: include_guard_
= package_ + "_" + file_->name() + "_H_";
Why this method pre-computing the include guard? Looks like you only need it in
one place when generating the prologue?
(Don't worry about printing the guard name in the final #endif comment, not
worth the code complexity of keeping the extra var)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:72:
UpperString(&include_guard_);
Use ToUpperASCII from base/strings

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:73:
StripString(&include_guard_, ".-", '_');
ditto

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:83: while
(stack.size() > 0) {
s/.size() > 0/! ... .empty())

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:95: string
GetCppClassName(const Descriptor* message) {
this seems to be a static function

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:101: void
PrintBoilerplate() {
I think this should be called:
GeneratePrologue() (it's also consistent with the epilogue below).
Boilerplate it's too generic. At the end you can claim that everything is
ultimately boilerplate.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:103: "//
Autogenerated. DO NOT EDIT.\n"
maybe say: autogenerated by //components/tracing/tools/proto_zero_plugin

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:133: break;
also TYPE_FLOAT

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:134: case
FieldDescriptor::TYPE_INT64:
I think this is SINT64.
It seems proto supports int64, signed int 64 and unsigned int64.
The diff between sint and int, IIRC, is that sint uses zig zag encoding while
int uses just the regular encoding and wastes space on negative numbers.
For the moment I'd just support UINT64 and add a TODO to deal with the rest
later.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:146: default:
Add TYPE_UINT32

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:152: " 
$appender$($number$, value);\n"
s/number/field_number

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:161: void
PrintNestedMessageField(const FieldDescriptor* field) {
For the moment let's leave this out and re-add once I have the base class.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:175: void
PrintFields(const Descriptor* message) {
I'd s/message/descriptor/ here and elsewhere. In general the pattern is that if
you call a name after a class and it is non ambiguous (you have only one
instance aroun) you call it like the class name but with lowercase.
Feels a bit weird for legibility to take an instance of Descriptor and call it
"message".
This reminds one of the first lessons of technical drawing in high school, where
our professor at some point came out saying "Let's take a point A and, hmmm,
let's call it B". Okay you can technically do that but... why? :)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:193:
PrintFields(message);
Legibility: hmm maybe PrintFields should just be inlined inside here. Feels like
this is splitting the code into too many hops.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:198: void
PrintEpilog() {
s/Epilog/Epilogue/

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:199: for (auto it =
namespaces_.rbegin(); it != namespaces_.rend(); ++it) {
for (const std::string& ns : namespaces_)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:203: "#endif  //
$guard$\n",
yeah don't worry about this (the guard name in the comment).

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:207: const
FileDescriptor* file_;
I'd s/file_/descriptor_/.
file_. suggests it's a unix file (or stdio FILE*), which is not the case :)

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
File components/tracing/proto_zero_plugin/proto_zero_generator.h (right):

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.h:10: #include
<google/protobuf/compiler/code_generator.h>
Can you make this an absolute include? i.e.
#include "third_party/protobuf/...."

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.h:12: namespace google
{
Is it required to put this into the google::protobuf namespace?
If not I'd just put under tracing::proto as I'm putting the rest there.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.h:17: class
LIBPROTOC_EXPORT Generator : public CodeGenerator {
not sure this needs to be _EXPORTed. 
This seems to be used only by proto_zero_plugin.cc, and I assume plugin.cc and
generator.cc are part of the same linker unit, right?

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.h:19: Generator() {}
Don't inline the ctor and dtor.
See
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.h:22: bool
Generate(const FileDescriptor* file,
Add a 
// CodeGenerator implementation.

It's the general pattern when you extend virt methods of a base class, to
explain where they come from.

https://codereview.chromium.org/2083373002/diff/20001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.h:26: };
DISALLOW_COPY_AND_ASSIGN

[kraynov, 2016-07-08 17:10:45]

Example of generated file (same as used for unit tests):

// Autogenerated. DO NOT EDIT.
// Protoc has generated these stubs with //components/tracing/proto_zero_plugin.

#ifndef FOO_BAR_MOCK_MESSAGES_PROTO_H_
#define FOO_BAR_MOCK_MESSAGES_PROTO_H_

#include <inttypes.h>

// Runtime is optional for testing purposes.
#ifndef PROTO_ZERO_NO_RUNTIME
#include "components/tracing/core/proto_zero_message.h"
#endif

namespace foo {
namespace bar {

class NestedA;
class NestedA_NestedB;
class NestedA_NestedB_NestedC;
class EveryField;

enum SmallEnum : int32_t {
  TO_BE = 1,
  NOT_TO_BE = 0,
};

enum SignedEnum : int32_t {
  POSITIVE = 1,
  NEUTRAL = 0,
  NEGATIVE = -1,
};

enum BigEnum : int32_t {
  BEGIN = 10,
  END = 100500,
};

enum EveryField_NestedEnum : int32_t {
  PING = 1,
  PONG = 2,
};

class NestedA : public ::tracing::proto::AppendOnlyProtoMessage {
 public:
  using NestedB = ::foo::bar::NestedA_NestedB;
  NestedA_NestedB* add_repeated_a() {
    NestedA_NestedB* nested;
    OpenNestedMessage(2, &nested);
    return nested;
  }
};

class NestedA_NestedB : public ::tracing::proto::AppendOnlyProtoMessage {
 public:
  using NestedC = ::foo::bar::NestedA_NestedB_NestedC;
  NestedA_NestedB_NestedC* set_value_b() {
    NestedA_NestedB_NestedC* nested;
    OpenNestedMessage(1, &nested);
    return nested;
  }
};

class NestedA_NestedB_NestedC : public ::tracing::proto::AppendOnlyProtoMessage
{
 public:
  void set_value_c(int32_t value) {
    AppendInt32(1, value);
  }
};

class EveryField : public ::tracing::proto::AppendOnlyProtoMessage {
 public:
  using NestedEnum = ::foo::bar::EveryField_NestedEnum;
  void set_field_int32(int32_t value) {
    AppendInt32(1, value);
  }
  void set_field_int64(int64_t value) {
    AppendInt64(2, value);
  }
  void set_field_uint32(uint32_t value) {
    AppendUint32(3, value);
  }
  void set_field_uint64(uint64_t value) {
    AppendUint64(4, value);
  }
  void set_field_sint32(int32_t value) {
    AppendSint32(5, value);
  }
  void set_field_sint64(int64_t value) {
    AppendSint64(6, value);
  }
  void set_field_fixed32(uint32_t value) {
    AppendFixed32(7, value);
  }
  void set_field_fixed64(uint64_t value) {
    AppendFixed64(8, value);
  }
  void set_field_sfixed32(int32_t value) {
    AppendSfixed32(9, value);
  }
  void set_field_sfixed64(int64_t value) {
    AppendSfixed64(10, value);
  }
  void set_field_float(float value) {
    AppendFloat(11, value);
  }
  void set_field_double(double value) {
    AppendDouble(12, value);
  }
  void set_field_bool(bool value) {
    AppendBool(13, value);
  }
  void set_small_enum(::foo::bar::SmallEnum value) {
    AppendTinyNumber(51, value);
  }
  void set_signed_enum(::foo::bar::SignedEnum value) {
    AppendInt32(52, value);
  }
  void set_big_enum(::foo::bar::BigEnum value) {
    AppendInt32(53, value);
  }
  void set_field_string(const char* value) {
    AppendString(500, value);
  }
  void set_field_bytes(const uint8_t* data, size_t size) {
    AppendBytes(505, data, size);
  }
  void set_nested_enum(::foo::bar::EveryField_NestedEnum value) {
    AppendTinyNumber(600, value);
  }
  void add_repeated_int32(int32_t value) {
    AppendInt32(999, value);
  }
};

} // namespace bar
} // namespace foo
#endif  // Include guard.

[Primiano Tucci (use gerrit), 2016-07-11 16:52:07]

ok overall looks great. I'll have some more comments but at this point it's
worth splitting this into the official CLs.
Let's have a first CL where you just generate empty files (or maybe just the
ifdef guard) and then gradually enrich that.
I'll add my comments there.
Also I'll prepare a design doc in the meantime.

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
File components/tracing/proto_zero_plugin/proto_zero_generator.cc (right):

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:19: //
MessageDescriptor is a longer name but has more obvious meaning.
It also makes the class harder to codesearch. I'd stick with MessageDescriptor
(so just using google::protobuf::Descriptor)

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:44: for (const
EnumDescriptor* enumeration: enums_) {
you can drop the braces for single line for loops (and if-s)

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:54: std::string
GetFirstError() {
not sure you want to return this by copy. I think this should be a:
const std::string& GetFirstError() const

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:94: // Scanning
pass.
This comment doesn't say anything more than "Prepare". Either drop it or say
what it does. I'd just drop it as you have comments below.
Also I'd probably call it "Preprocess"

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:138: "#include
<inttypes.h>\n\n"
s/inttypes/stdint/

https://codereview.chromium.org/2083373002/diff/60001/components/tracing/prot...
components/tracing/proto_zero_plugin/proto_zero_generator.cc:144: // Print
namespaces.
add a blank newline before these comments. It makes code easier to read as it
clarifies which line the code belongs to.