[PATCH] D14294: Add macro support to llvm-dwarfdump tool

[Alexey Samsonov via llvm-commits]

samsonov added a comment.

Looks mostly good. A few remaining comments below.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:24
@@ +23,3 @@
+  for (const Entry &E : Macros) {
+    IndLevel -= (E.Type == DW_MACINFO_end_file);
+    // Print indentation.
----------------
Do this only if IndLevel is positive

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:36
@@ +35,3 @@
+    case DW_MACINFO_define:
+    case dwarf::DW_MACINFO_undef:
+      OS << " - lineno: " << E.Line;
----------------
you don't use `dwarf::` namespace specified in another places, remove from here?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:78
@@ +77,3 @@
+      // 2. Source line
+      E.Line = (unsigned)data.getULEB128(&Offset);
+      // 3. Macro string
----------------
Why do you cast it to `unsigned`? `Line` is uint64_t.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:84
@@ +83,3 @@
+      // 2. Source line
+      E.Line = (unsigned)data.getULEB128(&Offset);
+      // 3. Source file id
----------------
see above

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:86
@@ +85,3 @@
+      // 3. Source file id
+      E.File = (uint16_t)data.getULEB128(&Offset);
+      break;
----------------
see above - why uint16_t?


Repository:
  rL LLVM

http://reviews.llvm.org/D14294