[PATCH] D56844: Breakpad: Extract parsing code into a separate file
Pavel Labath via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 21 12:36:10 PST 2019
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
More information about the llvm-commits
mailing list