[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();
   }
----------------
clayborg wrote:
> 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.
> If LineTable and Inline are optional is really complicated the operator == and operator < functions.

operator== should fall out automatically out of Optional's operator== implementation, same for operator<.

> 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.

Are you worried about an extra copy operation? Couldn't you still construct it in-place through RVO?

My meta point here is that we have an abstraction for things that may be invalid in LLVM (and C++17)  and that is Optional. When classes roll their own implementation of Optional via isValid() methods then users of these objects need to know about it and will almost inevitably forget the check validity at some point in the future, whereas Optional forces them to think about this when they write code that uses the class.


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

https://reviews.llvm.org/D66602