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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 7 inline comments as done.
labath added a comment.

Thanks for the review. I'll create another review with the changes stemming from this.



================
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.
----------------
lemo wrote:
> the comment is great, but I think we should still have symbolic constants for all these magic values
I just had an idea on how to remove the numbers altogether. I'll create a separate patch to see what you think.


================
Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:34
+
+  ~Record() = default;
+
----------------
lemo wrote:
> 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)
Yes, but they won't be able to polymorphically delete these objects through a base pointer, as the base destructor is protected.  With a protected destructor, the compiler wouldn't warn here even if the class had other virtual members.

(BTW, this is one of the reasons I gave up on a generic "parse" function - it'd force heap allocations for polymorphic treatment without any big benefit).


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

The reason I used the llvm naming convention here, is because this was a fresh class, which didn't have to interact with lldb at all. That's why I could (and wanted to) follow llvm coding practices as best as I could. Using lldb names for things felt weird here. 


================
Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:84
+
+class PublicRecord : public Record {
+public:
----------------
lemo wrote:
> 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, ...)
I thought about that too, but I couldn't really make up my mind. In the end I did this, but I can't say I had a good reason for it. I think I can change the members to be public, given that you had the same thought.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56844