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