[PATCH] D66602: Add a LineTable class to GSYM and test it.

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 3 inline comments as done.
clayborg added inline comments.


================
Comment at: include/llvm/DebugInfo/GSYM/FunctionInfo.h:46
     /// and we need to be able to tell which one is the better object to use.
-    return !Lines.empty() || Inline.isValid();
+    return !LineTable.empty() || Inline.isValid();
   }
----------------
aprantl wrote:
> I know I'm asking this at every patch, but: Why isn't this an Optional<LineTable> and why does it have to have a clear() method?
If LineTable and Inline are optional is really complicated the operator == and operator < functions. Also we populate a FunctionInfo object from debug info so it is nice to just use the objects that are in the FunctionInfo and not have to make a temp and move it into the FunctionInfo object. So it was to keep the code simpler. If you really want the optionals here I can add them, but it does make the operator functions less readable and harder to implement.


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:18
+enum LineTableOpCode {
+  LTOC_EndSequence = 0x00,  ///< End of the line table
+  LTOC_SetFile = 0x01,      ///< Set LineTableRow.file_idx, don't push a row
----------------
aprantl wrote:
> I think the LTOC_ prefix is redundant?
will remove.


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:123
+
+llvm::Error LineTable::encode(FileWriter &Out, uint64_t BaseAddr) const {
+  // Users must verify the LineTable is valid prior to calling this funtion.
----------------
aprantl wrote:
> Could this return an Expected<LineTable> instead?
This is the encoding function. It takes an existing LineTable objects and encodes it. The decode function is static and does return a Expected<LineTable>. So we only need an error back if it fails to encode right? 


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

https://reviews.llvm.org/D66602