[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:42
+  bool writeData(const void *Src, size_t SrcLen);
+  bool writeCStr(const char *CStr);
+  bool fixup32(uint32_t Value, off_t Offset);
----------------
aprantl wrote:
> clayborg wrote:
> > aprantl wrote:
> > > This probably shouldn't exist :-)
> > I believe it should. It clearly states we are writing a NULL terminated C string. 
> > 
> > If we pass a StringRef the simple cases are clear:
> > - StringRef("hello"): write out "hello\0"
> > - StringRef("")? Write out "\0"
> > 
> > But there are many things that are not as clear if we pass in:
> > - StringRef()? Write out nothing since it has no data? Or write out "\0"?
> > - StringRef("hello\0world\0")? Just write out "hello\0" since that is the first C string?
> > - StringRef("hello\0") (where size includes the NULL termination byte) write out "hello\0". We will need to check if the string ends with '\0', or contains any '\0' for so we avoid emitting an extra NULL character
> > 
> > 
> > 
> Let's ask the underlying question: Which of the above use-cases does a GSYM writer need, and why isn't the StringRef case sufficient?
ditto...


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:37
+  ~FileWriter();
+  /// writeU8 - Write a single uint8_t value into the stream at the current
+  /// file position.
----------------
`/// Write a ...`


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:40
+  ///
+  /// @param   Value The value to write into the stream.
+  void writeU8(uint8_t Value);
----------------
`/// \param Value`


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:88
+  /// is up to the user to ensure that Str doesn't contain any NULL characters
+  /// unless the addition NULL characters are desired.
+  ///
----------------
additional?


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

https://reviews.llvm.org/D63828