[Lldb-commits] [PATCH] D56590: breakpad: Add FUNC records to the symtab

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:26
 namespace {
 class LineIterator {
 public:
----------------
clayborg wrote:
> Move this functionality into llvm::breakpad::Line?
I haven't moved this part to the new file because (unlike everything else in that file) this depends on the ObjectFile class. Theoretically it can be moved to ObjectFileBreapad.h (or a new file) if needed, but ideally I'd like to avoid anyone else needing to parse the breakpad file contents. If the minidump process plugin for instance needed to access some of this information, I would have it go through the SymbolFile interface, which can present it in a nicer fashion.


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:232
+  };
+  for (llvm::StringRef line: lines(*m_obj_file, Token::Func)) {
+    // Here we can get either FUNC records (starting with FUNC), or line records
----------------
clayborg wrote:
> Should the iterator for Token::Func just return "FUNC" objects only? Maybe we add a Token::Line to the Token enumeration and then add an optional second parameter to the iterator? So any code that would want a "FUNC" and its lines, would do:
> 
> ```
> for (llvm::StringRef line: lines(*m_obj_file, Token::Func, Token::Line)) {
> }
> ```
With the new parse functions this should not be necessary as you can just say "try to parse this line as a FUNC record", and that will automatically reject LINE record as well as any other malformed lines. I think that would make sense if I made the iterator perform the parsing internally and return already parsed records.

I didn't do that (for now) because returning parse errors from an iterator gets weird, and it didn't seem worth the small amount of code it would save.


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

https://reviews.llvm.org/D56590