[PATCH] D27851: [DWARF] - Introduce DWARFDebugPubTable class for dumping pub* sections.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h:22
+
+// Represents structure for holding and parsing .debug_pub* tables.
+class DWARFDebugPubTable {
----------------
aprantl wrote:
> Please doxygen-ify these comments.
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h:58
+
+    std::vector<Entry> Entires;
+  };
----------------
davide wrote:
> s/Entires/Entries/ maybe?
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h:64-67
+
+  SetColl Sets;
+
+  bool GnuStyle;
----------------
davide wrote:
> Add comments to explain what `GnuStyle` means.
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h:73-74
+
+  entry_iterator_range entries() const {
+    return entry_iterator_range(Sets.begin(), Sets.end());
+  }
----------------
dblaikie wrote:
> Probably no need to worry about using a separate range type here - just return a const ref to 'Sets' directly?
Ok. I am returning ArrayRef now.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:201-217
   if (DumpType == DIDT_All || DumpType == DIDT_Pubnames)
-    dumpPubSection(OS, "debug_pubnames", getPubNamesSection(),
-                   isLittleEndian(), false);
+    DWARFDebugPubTable(getPubNamesSection(), isLittleEndian(), false)
+        .dump("debug_pubnames", OS);
 
   if (DumpType == DIDT_All || DumpType == DIDT_Pubtypes)
-    dumpPubSection(OS, "debug_pubtypes", getPubTypesSection(),
-                   isLittleEndian(), false);
+    DWARFDebugPubTable(getPubTypesSection(), isLittleEndian(), false)
+        .dump("debug_pubtypes", OS);
----------------
davide wrote:
> You can probably express this in a more compact form (and avoid some duplication) with a mapping from `DumpType` to the section name. Up to you.
I'll try to think how to improve that place and suggest a separate patch.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:35-37
+      uint8_t IndexEntryValue = 0;
+      if (GnuStyle)
+        IndexEntryValue = PubNames.getU8(&Offset);
----------------
davide wrote:
> `uint8_t IndexEntryValue = (GnuStyle) ? ... : 0;`
Done.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:53-55
+      OS << "Offset     Linkage  Kind     Name\n";
+    else
+      OS << "Offset     Name\n";
----------------
davide wrote:
> Maybe use the ternary operator? As in 
> ```OS << (GnuStyle ? ... : ...);```
Done.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:65
+           << format("%-8s",
+                     dwarf::GDBIndexEntryKindString(E.Descriptor.Kind).data())
+           << ' ';
----------------
davide wrote:
> ehm, is this clang formatted? Probably yes, but I'm double checking because it looks weird.
Yes, it was formatted. I rewrote this part a bit to look more straightforward,
hopefully it is better looking now.


Repository:
  rL LLVM

https://reviews.llvm.org/D27851