[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:13-15
+#include "lldb/lldb-defines.h"
+
+#include "llvm/ADT/Optional.h"
----------------



================
Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:108-138
+  /// Convert to boolean operator.
+  ///
+  /// This allows code to check a SourceLocationSpec object to see if it
+  /// contains anything valid using code such as:
+  ///
+  /// \code
+  /// SourceLocationSpec location_spec(...);
----------------
I guess this is  no longer needed now that the factory guarantees the spec is valid?


================
Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:183
+
+  bool HasColumn() const { return m_column.hasValue(); }
+
----------------
Is there any value to having this? I assume that if you want to know if there is a column, it's to use it after, so you might as well call get column and use the value if it's set. 


================
Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:195-196
+  llvm::Optional<uint16_t> m_column;
+  bool m_check_inlines;
+  bool m_exact_match;
+};
----------------
Maybe add a Doxygen comment to specify what these two mean exactly. 


================
Comment at: lldb/source/Utility/SourceLocationSpec.cpp:59-61
+bool SourceLocationSpec::operator!=(const SourceLocationSpec &rhs) const {
+  return !(*this == rhs);
+}
----------------
Isn't this the default implementation of `operator !=`? I think we can probably omit it? 


================
Comment at: lldb/unittests/Utility/SourceLocationSpecTest.cpp:96
+
+  // mutating FileSpec + const Inlined, ExactMatch, Line
+  EXPECT_TRUE(
----------------



================
Comment at: lldb/unittests/Utility/SourceLocationSpecTest.cpp:136-143
+  auto Create =
+      [](bool check_inlines, bool exact_match, FileSpec fs, uint32_t line,
+         uint16_t column = LLDB_INVALID_COLUMN_NUMBER) -> SourceLocationSpec {
+    auto location = SourceLocationSpec::Create(fs, line, column, check_inlines,
+                                               exact_match);
+    lldbassert(location && "Invalid SourceLocationSpec.");
+    return *location;
----------------
This looks identical to the `Create` above. If it is, make it a static function to avoid code duplication. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962