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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

The logic looks correct, some minor comments inline.



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


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


================
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);
----------------
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.


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


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


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:65
+           << format("%-8s",
+                     dwarf::GDBIndexEntryKindString(E.Descriptor.Kind).data())
+           << ' ';
----------------
ehm, is this clang formatted? Probably yes, but I'm double checking because it looks weird.


https://reviews.llvm.org/D27851