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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl 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();
   }
----------------
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?


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
----------------
Wrong license.


================
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
----------------
I think the LTOC_ prefix is redundant?


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:35
+
+static bool encode_special(int64_t MinLineDelta, int64_t MaxLineDelta,
+                           int64_t LineDelta, uint64_t AddrDelta,
----------------
LLVM wants camel case


================
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.
----------------
Could this return an Expected<LineTable> instead?


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

https://reviews.llvm.org/D66602