[PATCH] D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Hi Greg, sorry about the delay. I have a few small nits about documentation and one question about error handling in the `decode` method, but otherwise this looks good to me.



================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:25
+
+/// A simplified binary data writer class that doesn't require targets, target
+/// definitions, architectures, or require any other optional compile time
----------------
It seems like now the `FileWriter` is mostly concerned with the byte order. I'd update this comment saying that it's a wrapper around `raw_pwrite_stream` with convenience methods to deal with endianness.  


================
Comment at: include/llvm/DebugInfo/GSYM/Range.h:60
+  /// encoded addresses easy to relocate as we just need to relocate one base
+  /// address.
+  void decode(DataExtractor &Data, uint64_t BaseAddr, uint32_t &Offset);
----------------
Can you add an `@{` and `@}` around this to group the comment? (http://www.doxygen.nl/manual/grouping.html)


================
Comment at: include/llvm/DebugInfo/GSYM/Range.h:97
+  /// details.
+  void decode(DataExtractor &Data, uint64_t BaseAddr, uint32_t &Offset);
+  void encode(FileWriter &O, uint64_t BaseAddr) const;
----------------
Can you add an `@{` and `@}` around this to group the comment?


================
Comment at: lib/DebugInfo/GSYM/Range.cpp:71
+  Start = StartAddr;
+  End = StartAddr + Size;
+}
----------------
Should we check that the new offsets are valid? 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63828/new/

https://reviews.llvm.org/D63828