amccarth added a comment.
This looks good. I have a few questions inline, but none of those are major
concerns.
================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:173
+ llvm::DenseMap<llvm::StringRef, llvm::StringRef> UnwindRules;
+};
+
----------------
I'm not a fan of deep class hierarchies, but given that StackCFIRecord is a
subset of StackCFIInitRecord and given that the naming suggests
StackCFIInitRecord is a concrete type of StackCFIRecord, should
StackCFIInitRecord derive from StackCFIRecord? (Or perhaps contain one?)
If not, perhaps a comment to explain why not.
================
Comment at: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp:123
+ "STACK CFI 47 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+}
+
----------------
All of the negative parsing tests seem to be incomplete (e.g., truncated or a
missing `INIT`). Should there be others, like having the wrong token type? Or
an invalid token? Or one that has extra tokens at the end of an otherwise
valid record?
I see you're following the pattern for the other types here, but is that enough?
================
Comment at: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp:132
+ StackCFIRecord::parse("STACK CFI INIT 47 8 .cfa: $esp 4 +"));
+}
----------------
This test relies on the parsing test for the StackCFIInitRecord having covered
most of the cases. That works because the parsing implementation is shared, so
I'm OK with it. Will others be concerned that this test isn't as independent
as it could be?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60268/new/
https://reviews.llvm.org/D60268
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
