[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: 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.
----------------
MaskRay wrote:
> I think calling hasError() in every write function is less ideal. Errors can be checked after a batch write in the call sites.
So the question really becomes do we want the first error to stick, or just let the stream get into a bad state and then report some random error later. Error checking is quite cheap (a pointer check) so I don't think the overhead is too much. I would much rather know an accurate first error over just getting some "failed to write" error later when something else caused the error in the first place. Thoughts?


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:97
+    return; // Don't do anything if we have an error.
+  if (Data.empty())
+    return;
----------------
MaskRay wrote:
> Write on empty data just works. No need to check emptiness.
The main worry was passing nullptr for the data. 


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:131
+    return;
+  off_t AlignedOffset = (Offset + Align - 1) / Align * Align;
+  if (AlignedOffset == Offset)
----------------
MaskRay wrote:
> If `Align` is a power of 2, you can use `(Offset+Align-1) & -Align`.
It isn't required to be a power of 2 currently.


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

https://reviews.llvm.org/D63828