proto_zero_plugin [NOT FOR REVIEW].

components/tracing/proto_zero_plugin/proto_zero_generator.cc

Index: components/tracing/proto_zero_plugin/proto_zero_generator.cc
diff --git a/components/tracing/proto_zero_plugin/proto_zero_generator.cc b/components/tracing/proto_zero_plugin/proto_zero_generator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d7bd85eb066ea16cbe016cb58f44f548844d3d32
--- /dev/null
+++ b/components/tracing/proto_zero_plugin/proto_zero_generator.cc
@@ -0,0 +1,243 @@
+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "proto_zero_generator.h"
+
+#include <string>
+#include <algorithm>
+
+#include <google/protobuf/descriptor.h>

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

make these absolute includes

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

+#include <google/protobuf/io/printer.h>
+#include <google/protobuf/io/zero_copy_stream.h>
+#include <google/protobuf/stubs/strutil.h>
+
+namespace google {
+namespace protobuf {
+namespace compiler {
+namespace fastcpp {
+
+namespace {
+
+// TODO(kraynov) Ship "lite" runtime in order to use outside of tracing.

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

Not sure what this TODO means.

+const string OUT_INCLUDES =

[Primiano Tucci (use gerrit), 2016/07/05 11:30:16]

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.

+  "#include \"base/trace_event/append_only_proto_message.h\"\n";
+const string OUT_MESSAGE_CLASS =
+  "::base::trace_event::AppendOnlyProtoMessage";
+
+
+class GeneratorJob {
+ public:
+  GeneratorJob(const FileDescriptor* file, io::Printer* printer)
+    : file_(file), printer_(printer) {}
+
+  void Run() {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

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()?

+    PrepareNamespace();
+    PrepareIncludeGuard();
+    PrepareMessages();
+
+    PrintBoilerplate();

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

I'd s/Print/Generate/ in these functions.

+    for (const Descriptor* message : messages_) {
+      PrintMessage(message);
+    }
+    PrintEpilog();
+  }
+
+ private:
+  // Split string by dots.

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

Looks like this is reinventing base/strings/string_tokenizer.h. Just reuse that
(linking against base shouln't be a problem)

+  vector<string> StringTokens(const string& str) {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

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.

+    vector<string> tokens;
+    size_t left = 0;
+    while (left < str.size()) {
+      size_t right = std::min(str.find('.', left), str.size());
+      tokens.push_back(str.substr(left, right));
+      left = right + 1;
+    }
+    return tokens;
+  }
+
+  // Seal generated classes into namespace corresponding to package name.
+  void PrepareNamespace() {
+    package_ = file_->package();
+    namespaces_ = StringTokens(package_);
+    absolute_namespace_ = "::";
+    for (const string ns : namespaces_) {

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

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 &)

+      absolute_namespace_ += ns + "::";

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

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)

+    }
+  }
+
+  // Unique define to guard a header.
+  void PrepareIncludeGuard() {
+    include_guard_ = package_ + "_" + file_->name() + "_H_";

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

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)

+    UpperString(&include_guard_);

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

Use ToUpperASCII from base/strings

+    StripString(&include_guard_, ".-", '_');

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

ditto

+  }
+
+  // Collect all messages in DFS order.
+  // TODO(kraynov) Try topological sort to eliminate forward declarations.
+  void PrepareMessages() {
+    vector<const Descriptor*> stack;
+    for (int i = 0; i < file_->message_type_count(); ++i) {
+      stack.push_back(file_->message_type(i));
+    }
+    while (stack.size() > 0) {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:16]

s/.size() > 0/! ... .empty())

+      const Descriptor* message = stack.back();
+      stack.pop_back();
+      messages_.push_back(message);
+      for (int i = 0; i < message->nested_type_count(); ++i) {
+        stack.push_back(message->nested_type(i));
+      }
+    }
+  }
+
+  // Get C++ class name corresponding to message descriptor.
+  // Nested names are splitted by underscores.
+  string GetCppClassName(const Descriptor* message) {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

this seems to be a static function

+    string class_name = StripPrefixString(message->full_name(), package_ + ".");
+    StripString(&class_name, ".-", '_');
+    return class_name;
+  }
+
+  void PrintBoilerplate() {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:16]

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.

+    printer_->Print(
+        "// Autogenerated. DO NOT EDIT.\n"

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

maybe say: autogenerated by //components/tracing/tools/proto_zero_plugin

+        "#ifndef $guard$\n"
+        "#define $guard$\n\n"
+        "$includes$\n",
+
+        "includes", OUT_INCLUDES,
+        "guard", include_guard_);
+
+    // Print namespaces
+    for (string ns : namespaces_) {
+      printer_->Print("namespace $ns$ {\n", "ns", ns);
+    }
+    printer_->Print("\n");
+    // Print forward declarations
+    for (const Descriptor* message : messages_) {
+      printer_->Print(
+          "class $class$;\n",
+          "class", GetCppClassName(message));
+    }
+    printer_->Print("\n");
+  }
+
+  void PrintSimpleField(const FieldDescriptor* field) {
+    string type;
+    string appender;
+
+    switch (field->type()) {
+      case FieldDescriptor::TYPE_DOUBLE:
+        type = "double";
+        appender = "AppendDouble";
+        break;

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

also TYPE_FLOAT

+      case FieldDescriptor::TYPE_INT64:

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

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.

+        type = "int64_t";
+        appender = "AppendVarIntSigned";
+        break;
+      case FieldDescriptor::TYPE_BOOL:
+        type = "bool";
+        appender = "AppendVarIntUnsigned";
+        break;
+      case FieldDescriptor::TYPE_STRING:
+        type = "const char*";
+        appender = "AppendString";
+        break;
+      default:

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

Add TYPE_UINT32  

+        // TODO(kraynov) Fail if not supported.
+        break;
+    }
+    printer_->Print(
+        "void set_$name$($type$ value) {\n"
+        "  $appender$($number$, value);\n"

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

s/number/field_number

+        "}\n",
+
+        "type", type,
+        "name", field->name(),
+        "appender", appender,
+        "number", std::to_string(field->number()));
+  }
+
+  void PrintNestedMessageField(const FieldDescriptor* field) {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

For the moment let's leave this out and re-add once I have the base class.

+    printer_->Print(
+        "$class$* add_$name$() {\n"
+        "  $class$* inst = new $class$(buffer(), this);\n"
+        "  BeginNestedMessage($number$, std::unique_ptr<$base$>(inst));\n"
+        "  return inst;\n"
+        "}\n",
+
+        "class", absolute_namespace_ + GetCppClassName(field->message_type()),
+        "name", field->name(),
+        "number", std::to_string(field->number()),
+        "base", OUT_MESSAGE_CLASS);
+  }
+
+  void PrintFields(const Descriptor* message) {

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

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? :)

+    for (int i = 0; i < message->field_count(); ++i) {
+      if (message->field(i)->type() != FieldDescriptor::TYPE_MESSAGE) {
+        PrintSimpleField(message->field(i));
+      } else {
+        PrintNestedMessageField(message->field(i));
+      }
+    }
+  }
+
+  void PrintMessage(const Descriptor* message) {
+    printer_->Print(
+        "class $class$ : public $base$ {\n",
+
+        "class", GetCppClassName(message),
+        "base", OUT_MESSAGE_CLASS);
+
+    printer_->Indent();
+    PrintFields(message);

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

Legibility: hmm maybe PrintFields should just be inlined inside here. Feels like
this is splitting the code into too many hops.

+    printer_->Outdent();
+    printer_->Print("};\n\n");
+  }
+
+  void PrintEpilog() {

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

s/Epilog/Epilogue/

+    for (auto it = namespaces_.rbegin(); it != namespaces_.rend(); ++it) {

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

for (const std::string& ns : namespaces_)

+      printer_->Print("} // namespace $ns$\n", "ns", *it);
+    }
+    printer_->Print(""
+        "#endif  // $guard$\n",

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

yeah don't worry about this (the guard name in the comment).

+        "guard", include_guard_);
+  }
+
+  const FileDescriptor* file_;

[Primiano Tucci (use gerrit), 2016/07/05 11:30:17]

I'd s/file_/descriptor_/.
file_. suggests it's a unix file (or stdio FILE*), which is not the case :)

+  io::Printer* printer_;
+
+  string package_;
+  string include_guard_;
+  vector<string> namespaces_;
+  string absolute_namespace_;
+  vector<const Descriptor*> messages_;
+};
+
+}  // namespace
+
+bool Generator::Generate(const FileDescriptor* file,
+                         const string& parameter,
+                         GeneratorContext* context,
+                         string* error) const {
+
+  using google::protobuf::scoped_ptr;
+
+  string stub_name = StripSuffixString(file->name(), ".proto") + ".zeropb";
+
+  scoped_ptr<io::ZeroCopyOutputStream> pb_h(context->Open(stub_name + ".h"));
+  io::Printer pb_h_printer(pb_h.get(), '$');  // $ used as variable delimiter.
+  GeneratorJob job(file, &pb_h_printer);
+  job.Run();
+
+  scoped_ptr<io::ZeroCopyOutputStream> pb_cc(context->Open(stub_name + ".cc"));
+  io::Printer pb_cc_printer(pb_cc.get(), '$');
+  pb_cc_printer.Print("// This file intentionally left blank.\n");
+
+  return true;
+}
+
+}  // namespace fastcpp
+}  // namespace compiler
+}  // namespace protobuf
+}  // namespace google