[PATCH] D42297: [DebugInfo] Basic .debug_names dumping support

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks! I have a couple of stylistic suggestions inline.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:27
+
+class DWARFAcceleratorTable {
+protected:
----------------
Could you add a Doxygen comment explaining the purpose of this class please?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:176
+    dwarf::Tag Tag;
+    std::vector<std::pair<dwarf::Index, dwarf::Form>> Indices;
+
----------------
Would a struct with named members make the code more readable than a std::pair?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:183
+
+  struct Entry {
+    const Abbrev &Abbr;
----------------
Could you please add some one-liner Doxygen comments explaining what these inner structs  and classes are for?


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:25
 
+static std::string formatEnum(StringRef (*StringFn)(unsigned), StringRef Type, unsigned Value) {
+  std::string Str = StringFn(Value);
----------------
Would it make more sense for these functions to take a raw_ostream as first argument instead of creating a temporary std::string?


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:440
+
+std::pair<uint32_t, uint32_t> DWARFDebugNames::NameIndex::getName(uint32_t Index) const {
+  uint32_t StringOffsetOffset = StringOffsetsBase + 4 * (Index - 1);
----------------
A pair of two uint32_t looks dangerous to my eyes. Perhaps define a struct with two named members to avoid accidental confusion?


Repository:
  rL LLVM

https://reviews.llvm.org/D42297