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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:29-48
+Token breakpad::toToken(llvm::StringRef str) {
   return llvm::StringSwitch<Token>(str)
       .Case("MODULE", Token::Module)
       .Case("INFO", Token::Info)
       .Case("FILE", Token::File)
       .Case("FUNC", Token::Func)
       .Case("PUBLIC", Token::Public)
----------------
Move to BreakpadLine.cpp/.h?


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:20-23
+enum class Token { Unknown, Module, Info, File, Func, Public, Stack };
+
+Token toToken(llvm::StringRef str);
+llvm::StringRef toString(Token t);
----------------
Should we have a "Line" class in lldb_private::breakpad::Line? All functions that parse a breakpad line could be in this new class? Seeing as we have symbol files, object files and the process plug-in might need to parse these lines, seems like it would be cleaner? Maybe in BreakpadLine.cpp/.h? 

```
namespace lldb_private {
namespace breakpad {
class Line {
  enum class Token { Unknown, Module, Info, File, Func, Public, Stack };
  Line(llvm::StringRef s);
  Token parseToken();
  static llvm::StringRef tokenToString(Token t);
};
```


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:26
 namespace {
 class LineIterator {
 public:
----------------
Move this functionality into llvm::breakpad::Line?


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:187
+
+  auto parse = [&](llvm::StringRef line, bool is_func) {
+    // [m] address {size} param_size name
----------------
change "llvm::StringRef line" to "lldb_private::breakpad::Line" and remove second parameter as the "Line" class can know its token type?


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:233-240
+    // Here we can get either FUNC records (starting with FUNC), or line records
+    // (starting with a hex number).
+    llvm::StringRef token_str;
+    std::tie(token_str, line) = getToken(line);
+    if (toToken(token_str) != Token::Func)
+      continue; // Skip line records.
+
----------------
If we make a Line class this code would become:

```
lldb_private::breakpad::Line bp_line(line);
if (bp_line.GetToken() != Token::Func)
  continue;
parse(bp_line);
```



================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:246
+    // skip PUBLIC keyword
+    parse(getToken(line).second, false);
+  }
----------------
If we make a Line class this code would become:

```
parse(bp_line);
```



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

https://reviews.llvm.org/D56590