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

Leonard Mosescu via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Jan 18 11:17:50 PST 2019

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
+  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 {
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, ...)




More information about the lldb-commits mailing list