tracing v2: Add proto_utils.h

components/tracing/core/proto_utils.h

Index: components/tracing/core/proto_utils.h
diff --git a/components/tracing/core/proto_utils.h b/components/tracing/core/proto_utils.h
new file mode 100644
index 0000000000000000000000000000000000000000..01b7e4e3fa8452a62934aa02e9af13246f210e3e
--- /dev/null
+++ b/components/tracing/core/proto_utils.h
@@ -0,0 +1,88 @@
+// Copyright 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.
+
+#ifndef COMPONENTS_TRACING_CORE_PROTO_UTILS_H_
+#define COMPONENTS_TRACING_CORE_PROTO_UTILS_H_
+
+#include <inttypes.h>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "components/tracing/tracing_export.h"
+
+namespace tracing {
+namespace v2 {
+namespace proto {
+
+// See https://developers.google.com/protocol-buffers/docs/encoding wire types.
+
+enum : uint32_t {
+  kFieldTypeVarInt = 0,
+  kFieldTypeFixed64 = 1,
+  kFieldTypeLengthDelimited = 2,
+  kFieldTypeFixed32 = 5,
+};
+
+// Variable-length field types: (s)int32, (s)int64, bool, enum.
+inline uint32_t MakeTagVarInt(uint32_t field_id) {

[petrcermak, 2016/07/07 09:46:59]

Does it really make sense to define this for each type? It seems that defining a
generic function would be just as good:

inline uint32_t MakeTag(unit32_t field_type, uint32_t field_id) {
  return (field_id << 3) | field_type;
}

The calls would have almost the same length and be equally readable in my
opinion:

MakeTagLengthLimited(1234);
MakeTag(kFieldTypeLengthLimited, 1234);

I suppose it would then make sense to name the enum so that the compiler could
check the type of the |field_type|.

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

So I conceptually agree. In practice the only reason why I didn't make that in
the first place is because I am not 100% sure that all compilers will do const
propagation and inline this method as it should. I don't have bandwith to deal
with compiler optimizations right now (but I plan to do that later), and if it
turns out that it's not the case, optimize these is easier than rewriting the
code in the caller.

+  return (field_id << 3) | kFieldTypeVarInt;
+}
+
+// Length-limited field types: string, bytes, embedded messages.
+inline uint32_t MakeTagLengthLimited(uint32_t field_id) {
+  return (field_id << 3) | kFieldTypeLengthDelimited;
+}
+
+// 32-bit fixed-length field types: fixed32, sfixed32, float.
+inline uint32_t MakeTagFixed32(uint32_t field_id) {
+  return (field_id << 3) | kFieldTypeFixed32;
+}
+
+// 64-bit fixed-length field types: fixed64, sfixed64, double.
+inline uint32_t MakeTagFixed64(uint32_t field_id) {
+  return (field_id << 3) | kFieldTypeFixed64;
+}
+
+template <typename T>
+inline uint8_t* WriteVarIntInternal(T value, uint8_t* target) {
+  while (value >= 0x80) {
+    *target = static_cast<uint8_t>(value | 0x80);
+    value >>= 7;
+    ++target;
+  }
+  *target = static_cast<uint8_t>(value);
+  return target + 1;
+}
+
+inline uint8_t* WriteVarIntU32(uint32_t value, uint8_t* target) {

[petrcermak, 2016/07/07 09:46:59]

Again, does it really make sense to define these? I think that calling the
templated function would be equally readable. Furthermore, this would simplify
the testing code (see my comment below).

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

Hmm here the reality is that due to the TODO(kraynov) below I am not sure how
the templated WriteVarInt will evolve, and I don't want to touch the caller code
later too much.

+  return WriteVarIntInternal<uint32_t>(value, target);
+}
+
+inline uint8_t* WriteVarIntU64(uint64_t value, uint8_t* target) {
+  return WriteVarIntInternal<uint64_t>(value, target);
+}
+
+// TODO(kraynov): add support for signed integers and zig-zag encoding.
+
+// Writes a fixed-size redundant encoding of the given |value|. This is
+// used to backfill fixed-size reservations for the length field using a
+// non-canonical varint encoding (e.g. \x81\x80\x80\x00 instead of \x01).
+// See https://github.com/google/protobuf/issues/1530.
+// Concretely this is used when writing a nested message, where the size of

[petrcermak, 2016/07/07 09:46:59]

nit: comma after "Concretely". I'd personally prefer "In particular, ..."

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

Done.

+// nested message is not known until it is ended. In this case a fixed amount of

[petrcermak, 2016/07/07 09:46:59]

nit: comma after "In this case"

[petrcermak, 2016/07/07 09:46:59]

supernit: s/ended/finished/

[petrcermak, 2016/07/07 09:47:00]

nit: s/nested/the nested/

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

not sure about this but... done.

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

Done.

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

reworded all this a bit.

+// bytes are reserved to encode its size field and backfilled at the end.
+void WriteRedundantVarIntUnsigned(uint32_t value, size_t length, uint8_t* buf) {

[petrcermak, 2016/07/07 09:46:59]

Should the name be "VarInt" whe it's fixed-size? Also, I think it should be
"U32" instead of "Unsigned" to be consistent with WriteVarIntU32 et al.

[Primiano Tucci (use gerrit), 2016/07/07 12:59:25]

Yeah I know it's an odd concept, but this is truly a "redundant varint". I don't
want to call it fixed, beause that would be confusing w.r.t. fixed integer
encoding in proto.
Agree with this. Done

+  for (size_t i = 0; i < length; ++i) {
+    const uint8_t msb = (i < length - 1) ? 0x80 : 0;
+    buf[i] = static_cast<uint8_t>((value & 0x7F) | msb);
+    value >>= 7;
+  }
+  DCHECK_EQ(0u, value) << "Buffer too short to encode the given value";
+}
+
+}  // namespace proto
+}  // namespace v2
+}  // namespace tracing
+
+#endif  // COMPONENTS_TRACING_CORE_PROTO_UTILS_H_