[PATCH] D38409: [dwarfdump] Add -lookup option

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/DebugInfo/DIContext.h:60
+    DILineInfo Default;
+    return (*this) != Default;
+  }
----------------
`return *this != DILineInfo();` ?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:263
+  /// given address where applicable.
+  std::tuple<DWARFCompileUnit *, DWARFDie, DWARFDie>
+  getDIEsForAddress(uint64_t Address);
----------------
use a struct with named fields for better readability?

I assume this doesn't make sense as an optional because either of the fields may be not available and all are nullable. Yes, that seems ok.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:332
 
+  /// getSubroutineForAddress - Returns subprogram DIE with address range
+  /// encompassing the provided address. The pointer is alive as long as parsed
----------------
remove `getSubroutineForAddress -`.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:749
+    for (auto Child : DIE) {
+      Worklist.push_back(Child);
+    }
----------------
`Worklist.insert(Worklist.end(), DIE.children_begin(), DIE.children_end())`?

hmm... this maybe looks worse actually.



================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:247
 
+static bool lookup(DWARFContext &DICtx, uint64_t Address, raw_ostream &OS) {
+  auto Tuple = DICtx.getDIEsForAddress(Lookup);
----------------
repeat the desc int the doxygen comment so it is more clear what the function is supposed to do?


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:263
+
+  if (DILineInfo LineInfo = DICtx.getLineInfoForAddress(Lookup)) {
+    OS << "\nLine table ";
----------------
should this code live in a DILineInfo::dump() function?


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:284
+  // Handle the --lookup option.
+  if (Lookup) {
+    return lookup(DICtx, Lookup, OS);
----------------
extra {}


Repository:
  rL LLVM

https://reviews.llvm.org/D38409