tracing v2: building blocks for reading-back trace protobufs

components/tracing/core/proto_utils.cc

+// 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.
+
+#include "components/tracing/core/proto_utils.h"

[petrcermak, 2016/09/08 16:13:25]

Is there a particular reason for this import ordering? If not, I would expect
"components" to be after "base".

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

yes:
the first include is the corresponding .h file
then system includes
then chrome includes

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

+
+#include <limits>
+
+#include "base/sys_byteorder.h"
+
+#define CHECK_PTR_LE(a,b) CHECK_LE(reinterpret_cast<const void*>(a), \
+                                   reinterpret_cast<const void*>(b))
+namespace tracing {
+namespace v2 {
+namespace proto {
+
+const uint8_t* ParseVarInt(const uint8_t* start,
+                           const uint8_t* end,
+                           uint64_t* value) {
+  const uint8_t* pos = start;
+  uint64_t shift = 0;
+  *value = 0;
+  do {
+    CHECK_PTR_LE(pos, end - 1);
+    DCHECK_LT(shift, 64ull);
+    *value |= static_cast<uint64_t>(*pos & 0x7f) << shift;
+    shift += 7;
+  } while (*pos++ & 0x80);

[petrcermak, 2016/09/08 16:13:25]

for readability, maybe add parentheses: *(pos++)

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

There are 260 cases of *xx++ [1] vs 56 of *(xx++) [2]
majority wins :)

[1]
https://cs.chromium.org/search/?q=%5C*%5Cw%2B%5C%2B%5C%2B+-f:third_party&sq=p...
[2]
https://cs.chromium.org/search/?q=%5C*%5C(%5Cw%2B%5C%2B%5C%2B%5C)+-f:third_pa...

+  return pos;
+}
+
+const uint8_t* ParseField(const uint8_t* start,
+                          const uint8_t* end,
+                          uint32_t* field_id,
+                          FieldType* field_type,
+                          uint64_t* field_intvalue) {
+  const uint8_t* pos = start;
+  CHECK_LT(pos, end);

[petrcermak, 2016/09/08 16:13:25]

I know that you can use CHECK_LT here, but maybe use CHECK_PTR_LE for
consistency?

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

Done.

+  *field_type = static_cast<FieldType>(*pos & 7);

[oystein (OOO til 10th of July), 2016/09/08 21:26:04]

Hmm, the code embeds a lot of magic numbers here. Could we move some of these to
named constants, to make things a little more readable?

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

ok done and added some comments

+
+  uint64_t raw_field_id;
+  pos = ParseVarInt(pos, end, &raw_field_id);
+  raw_field_id >>= 3;

[petrcermak, 2016/09/08 16:13:25]

It took me a while to understand what this is for. Could you add a comment
explaining the fact that you shift by 3 because the 3 LSB are in *field_type?

[oystein (OOO til 10th of July), 2016/09/08 21:26:04]

Maybe use the same named constant in both cases to make it extra clear. More
math but clearer perhaps?

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

ok ok used more names. Just Stockholm syndrome after a month workin on protos.
To me that "3" was perfectly clear and unambiguous. I see that might not be the
case in general though :P

+
+  DCHECK_LE(raw_field_id, std::numeric_limits<uint32_t>::max());
+  *field_id = static_cast<uint32_t>(raw_field_id);
+
+  switch (*field_type) {
+    case kFieldTypeFixed64: {
+      CHECK_PTR_LE(pos + 8, end);
+      memcpy(field_intvalue, pos, 8);

[oystein (OOO til 10th of July), 2016/09/08 21:26:04]

sizeof(uint64_t) for clarity?

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

Done.

+      *field_intvalue = base::ByteSwapToLE64(*field_intvalue);
+      pos += 8;

[oystein (OOO til 10th of July), 2016/09/08 21:26:04]

Ditto

[Primiano Tucci (use gerrit), 2016/09/09 14:15:49]

Done.

+      break;
+    }
+    case kFieldTypeFixed32: {
+      CHECK_PTR_LE(pos + 4, end);

[oystein (OOO til 10th of July), 2016/09/08 21:26:04]

Ditto for uint32_t

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

Done.

+      uint32_t tmp;
+      memcpy(&tmp, pos, 4);
+      *field_intvalue = base::ByteSwapToLE32(tmp);
+      pos += 4;
+      break;
+    }
+    case kFieldTypeVarInt:
+    case kFieldTypeLengthDelimited: {
+      pos = ParseVarInt(pos, end, field_intvalue);
+      if (*field_type == kFieldTypeLengthDelimited) {

[petrcermak, 2016/09/08 16:13:25]

I think that readability might be worth the one duplicated line here:

  case kFieldTypeVarInt:
    pos = ParseVarInt(pos, end, field_intvalue);
    break;

  case kFieldTypeLengthDelimited:
    ...

[Primiano Tucci (use gerrit), 2016/09/09 14:15:48]

right makes sense. after various edits didn't ralize this is saving just one
line.

+        pos += *field_intvalue;
+        CHECK_PTR_LE(pos, end);
+      }
+      break;
+    }
+    default:
+      NOTREACHED() << "Unsupported proto field type " << *field_type;
+  }
+  return pos;
+}
+
+}  // namespace proto
+}  // namespace v2
+}  // namespace tracing