[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:55
+public:
+  void clear();
+  /// Extract an entire table, including all addresses.
----------------
Nit: a newline between these functions would improve readability, especially with the doxygen comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:63
+  uint8_t getAddrSize() const { return HeaderData.AddrSize; }
+  void dump(raw_ostream &OS, DIDumpOptions DumpOpts) const;
+
----------------
Please give the `DIDumpOptions` a default value to make the argument optional. (i.e. `DIDumpOptions DumpOpts = {}`)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:66
+  /// Return the address based on a given index.
+  Expected<uint64_t> getAddrEntry(uint32_t Index) const {
+    if (Index < Addrs.size())
----------------
Does this have to be implemented in the header?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1619
+uint8_t DWARFContext::getCUAddrSize() {
+  // In fact, different compile units may have different address byte
+  // sizes, but for simplicity we just use the address byte size of the
----------------
Nit: `s/fact/theory/`


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:120
+
+  // TODO: add support for non-zero SegSize
+  if (HeaderData.SegSize != 0)
----------------
Please use a full sentences here. 


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:155
+  static const char *Fmt64 = "0x%16.16" PRIx64;
+  std::string AddrFmt = "\n";
+  std::string AddrFmtVerbose = " => ";
----------------
I think a `raw_string_ostream` might better communicate your intentions of building up a string. 


https://reviews.llvm.org/D49676