[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This looks good to me, save for a couple comment nits.



================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:155
+    //
+    // TODO: revisit this check since it fires for apparently valid PDBs
+    //
----------------
Every apparently valid PDB or certain ones?  If it's particular ones, then perhaps we should create a bug report with a repro to make it easier to investigate in the future.


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:133
+    // TODO: If the file isn't a PE/COFF executable, look for the PDB in the
+    //  current directly. This provides a basic solution for debugging minidumps
+    //  although it's only a stop-gap until we implement a proper SymbolVendor
----------------
Did you mean "directory" instead of "directly"?

Also, this is worded as a TODO, but it describes the current situation.  Describing the current situation is fine, but I'd like the TODO to more clearly say what's do be done.  I gather that's "implement a proper SymbolVendor."


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142