[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

[Adrian McCarthy via Phabricator via lldb-commits]

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