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

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 3 inline comments as done.
clayborg 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);
----------------
aprantl wrote:
> This whole interface needs Doxygen comments.
Agreed, but I am not going to do it before we agree on the API.

I just didn't know if this class was going to be thrown out in favor of the MC layer. I couldn't figure out how to use it for raw files so I made this class, but I figured someone with more llvm expertise might let me know how to do it or how to do it another way.

I figured FileWriter would need changes so I just want to iterate on this and get to the "lgtm once we have doxygen comments", then I will add full docs. Is that ok? 


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:35
+  ~FileWriter();
+  bool writeU8(uint8_t Value);
+  bool writeU16(uint16_t Value);
----------------
aprantl wrote:
> What are the bool return values indicating? If it's error handling — is there a better way of doing this?
bool for success. We can remove this and make it void, or match what the MC layer does. Do we want to just switch all return values to "void" an dhave the user periodically check in this the FileWriter class and ask if it is good? Error checking on each write seems like overkill, so I tried to keep it simple with a bool for now. Let me know what you think.


================
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);
----------------
aprantl wrote:
> 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);
> 
> ?
I can just switch this to StringRef and have the user be on the hook for not passing in a StringRef with embedded NULL characters. We can always write the entire string and always add a NULL. Is that ok?


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

https://reviews.llvm.org/D63828