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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> It currently uses std::ostream over llvm::raw_ostream because llvm::raw_ostream doesn't support seeking which is required when encoding and decoding GSYM data.

There is raw_fd_ostream::seek. You just need to make sure `raw_fd_ostream::supportsSeeking()` returns true when you open it.

#include <iostream> is Forbidden <https://llvm.org/docs/CodingStandards.html#id49>.



================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:95
+void FileWriter::writeData(llvm::ArrayRef<uint8_t> Data) {
+  if (hasError())
+    return; // Don't do anything if we have an error.
----------------
I think calling hasError() in every write function is less ideal. Errors can be checked after a batch write in the call sites.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:97
+    return; // Don't do anything if we have an error.
+  if (Data.empty())
+    return;
----------------
Write on empty data just works. No need to check emptiness.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:131
+    return;
+  off_t AlignedOffset = (Offset + Align - 1) / Align * Align;
+  if (AlignedOffset == Offset)
----------------
If `Align` is a power of 2, you can use `(Offset+Align-1) & -Align`.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:136
+  std::string PadBytes(PadCount, '\0');
+  writeData(ArrayRef<uint8_t>(
+      reinterpret_cast<const uint8_t *>(PadBytes.data()), PadBytes.size()));
----------------
If you use raw_fd_ostream, there is `raw_ostream::write_zeros`.


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

https://reviews.llvm.org/D63828