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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
