[PATCH] D32713: [DWARFv5] Parse new line-table header format.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

seems generally fine, couple of stylistic nits inline.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:46
     uint16_t Version;
+    // In v5, size in bytes of an address (or segment offset).
+    uint8_t AddressSize;
----------------
These should probably all be `///`. Perhaps in a separate commit first? (no need to go through phabricator)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:30
 typedef DILineInfoSpecifier::FileLineInfoKind FileLineInfoKind;
+typedef std::pair<unsigned, unsigned> ContentDescriptor;
 
----------------
Would a struct with named members be more readable?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:84
+static void
+parseOldDirFileTables(DataExtractor debug_line_data, uint32_t *offset_ptr,
+                      uint64_t end_prologue_offset,
----------------
If this is accurate, I would prefer calling this `parseV2DirFileTables`


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:85
+parseOldDirFileTables(DataExtractor debug_line_data, uint32_t *offset_ptr,
+                      uint64_t end_prologue_offset,
+                      std::vector<const char *> &IncludeDirectories,
----------------
As a general comment, none of the variable/parameter names follow the LLVM coding style (capitalization).


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:89
+  while (*offset_ptr < end_prologue_offset) {
+    const char *s = debug_line_data.getCStr(offset_ptr);
+    if (s && s[0])
----------------
not super important, but LLVM style would be
```
if (!s || !s[0])
      break;
IncludeDirectories.push_back(s);
```


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:98
+    const char *name = debug_line_data.getCStr(offset_ptr);
+    if (name && name[0]) {
+      DWARFDebugLine::FileNameEntry fileEntry;
----------------
same here


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:133
+  // Get the directory entry description.
+
+  SmallVector<ContentDescriptor, 4> Descriptors;
----------------
extra newline


https://reviews.llvm.org/D32713