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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:34
+  FileWriter(std::ostream &S, llvm::support::endianness B) : OS(S), ByteOrder(B) {}
+  ~FileWriter();
+  bool writeU8(uint8_t Value);
----------------
This whole interface needs Doxygen comments.


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:35
+  ~FileWriter();
+  bool writeU8(uint8_t Value);
+  bool writeU16(uint16_t Value);
----------------
What are the bool return values indicating? If it's error handling — is there a better way of doing this?


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:43
+  bool writeData(llvm::ArrayRef<uint8_t> Data);
+  bool writeCStr(const char *CStr);
+  bool fixup32(uint32_t Value, off_t Offset);
----------------
clayborg wrote:
> It needs both the ability to write raw data (for raw data after being swapped and for internal implementation) and NULL terminated C strings (for string table).
Now that you changed the other API to take an ArrayRef, there should be no problem changing this method to:

bool writeNullTerminated(StringRef s);

?


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:12
+#include "llvm/Support/LEB128.h"
+#include <assert.h>
+
----------------
#include <cassert>


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

https://reviews.llvm.org/D63828