[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:184
+  FileSpec m_file_spec;
+  uint32_t m_line;
+  llvm::Optional<uint16_t> m_column;
----------------
mib wrote:
> JDevlieghere wrote:
> > Are there situations where the line is optional too? 
> Technically, `m_line` could be equal to 0 (if there is no line information) but that would basically mean this class is a `FileSpec` container.
> 
> I guess I can add an assert in the constructor checking if it's not null.
Indeed. Another option would be to make it a precondition that a SourceLocationSpec is always valid, assuming that unlike the column number, the file and line are usually valid. One way to do that is by adding asserts to the ctor or if this class is often created from user input, then I'd make the ctor private and add a static `CreateSoureLocationSpec` factory, that checks our preconditions and returns an Expected/Optional. That has the advantage of not having the change all the call sites for the few scenarios where those are actually invalid. This has the advantage of paying the price upfront when creating the LocationSpec rather than having to check in every place it's used. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962