[Lldb-commits] [PATCH] D56844: Breakpad: Extract parsing code into a separate file

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Looks good. A few questions/suggestions inline.



================
Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:74
+  static_assert(sizeof(data) == 20, "");
+  // The textual module id encoding should be between 33 and 40 bytes long,
+  // depending on the size of the age field, which is of variable length.
----------------
the comment is great, but I think we should still have symbolic constants for all these magic values


================
Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:34
+
+  ~Record() = default;
+
----------------
should this be virtual? (even though the class doesn't have other virtual members, the class hierarchy introduces the possibility for consumers to treat them a polymorphic types - ex. storing as Record* and use the kind type to figure out the concrete type)


================
Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:40
+private:
+  Kind TheKind;
+};
----------------
Just curious, what is the definitive convention for naming data members? A lot of LLDB code uses the m_camelCase convention.


================
Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:84
+
+class PublicRecord : public Record {
+public:
----------------
most of these types are just plain data containers, so why not make them structs? (and avoid all the boilerplate associated with public accessors, repetitive constructors, ...)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56844/new/

https://reviews.llvm.org/D56844