[lldb/Core] Add SourceLocationSpec class (NFC)

[mib]

Summary

A source location specifier class that holds a Declaration object containing
a FileSpec with line and column information. The column line is optional.
It also holds search flags that can be fetched by resolvers to look inlined
declarations and/or exact matches.

It describes a specific location in a source file and allows the user
to perform checks and comparaisons between multiple instances of that class.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

lldb/include/lldb/Utility/SourceLocationSpec.h

12–19

90

126

The class looks mostly immutable (which is good) so I think we can omit this.

153

160

Would it make sense to merge this into Equal and replace the bool with an enum value that represents

• file + line must match,
• file + line + col must match,
• file + line + col must match if lhs has it?

178

If 0 is not a valid column, we should enforce that rather than having two ways of representing it. Can we add an assert to the ctor and make it a precondition for the class?

184

Are there situations where the line is optional too?

lldb/unittests/Utility/SourceLocationSpecTest.cpp

1–2

[mib]

lldb/include/lldb/Utility/SourceLocationSpec.h

184

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.

[JDevlieghere]

lldb/include/lldb/Utility/SourceLocationSpec.h

184

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.

[mib]

lldb/include/lldb/Utility/SourceLocationSpec.h

160

I simplified the API removing the Match method since it's the same as calling Exact with bool full = false.

[mib]

Addressed @JDevlieghere comments.

[mib]

• Move boolean attributes to the end of the class.
• Fix deleted default constructor issue.

[JDevlieghere]

lldb/include/lldb/Utility/SourceLocationSpec.h

14–16

109–139

I guess this is no longer needed now that the factory guarantees the spec is valid?

184

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.

196–197

Maybe add a Doxygen comment to specify what these two mean exactly.

lldb/source/Utility/SourceLocationSpec.cpp

60–62

Isn't this the default implementation of operator !=? I think we can probably omit it?

lldb/unittests/Utility/SourceLocationSpecTest.cpp

97

137–144

This looks identical to the Create above. If it is, make it a static function to avoid code duplication.

[mib]

lldb/source/Utility/SourceLocationSpec.cpp

60–62

No, it has to be implemented separately.

[mib]

Address @JDevlieghere comments.

[JDevlieghere]

LGTM

[mib]

Fix unit test failure.

[mib]

Group the FileSpec, the Line and Column number into a Declaration.
Remove factory function to use constructor and added an IsValid method (and boolean operator)

[mib]

Wrap SourceLocationSpec column argument into an llvm::Optional

[JDevlieghere]

LGTM

lldb/unittests/Utility/CMakeLists.txt

42

Let's exclude the change to this file from this patch.

[JDevlieghere]

lldb/source/Core/SourceLocationSpec.cpp

48–61 ↗

(On Diff #341752)

As this is only using members form Declaration, should it be implemented there?

72–77 ↗

(On Diff #341752)

Looks like Declaration has a Dump method, so I guess this should just call that.

[mib]

Address @JDevlieghere comments.

[JDevlieghere]

A few more comments about things I missed in the previous review.

lldb/source/Core/SourceLocationSpec.cpp

57 ↗

(On Diff #341774)

Should this take a StreamString so we can write to the stream directory? Or alternatively change the dump method in declaration to also take a raw_ostream? Both would also improve the operator<<.

65–71 ↗

(On Diff #341774)

This should probably return a std::string instead. Now we're going to keep the string around forever for every invocation of this method.

lldb/include/lldb/Utility/SourceLocationSpec.h

+//===-- SourceLocationSpec.h ------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_SOURCELOCATIONSPEC_H
+#define LLDB_UTILITY_SOURCELOCATIONSPEC_H
+
+#include <string>
+
+#include "lldb/Utility/FileSpec.h"
+
+#include "llvm/ADT/Optional.h"
+
+#include <stddef.h>
+#include <stdint.h>
+
+namespace lldb_private {
+
+/// \class SourceLocationSpec SourceLocationSpec.h
+/// "lldb/Utility/SourceLocationSpec.h" A source location utility class.
+///
+/// A source location specifier class that holds a FileSpec object with line and
+/// column information. The column line is optional.
+class SourceLocationSpec {
+public:
+  SourceLocationSpec() = delete;
+
+  /// Constructor.
+  ///
+  /// Takes a \a file_spec with a \a line number and a \a column number. If
+  /// \a column is null or not provided, it is set to llvm::None.
+  ///
+  /// \param[in] file_spec
+  ///     The full or partial path to a file.
+  ///
+  /// \param[in] line
+  ///     The line number in the source file.
+  ///
+  ///  \param[in] column
+  ///     The column number in the line of the source file.
+  ///
+  ///
+  explicit SourceLocationSpec(FileSpec file_spec, uint32_t line);
+  explicit SourceLocationSpec(FileSpec file_spec, uint32_t line,
+                              uint16_t column);
+
+  /// Equal to operator
+  ///
+  /// Tests if this object is equal to \a rhs.
+  ///
+  /// \param[in] rhs
+  ///     A const SourceLocationSpec object reference to compare this object
+  ///     to.
+  ///
+  /// \return
+  ///     \b true if this object is equal to \a rhs, \b false
+  ///     otherwise.
+  bool operator==(const SourceLocationSpec &rhs) const;
+
+  /// Not equal to operator
+  ///
+  /// Tests if this object is not equal to \a rhs.
+  ///
+  /// \param[in] rhs
+  ///     A const SourceLocationSpec object reference to compare this object
+  ///     to.
+  ///
+  /// \return
+  ///     \b true if this object is equal to \a rhs, \b false
+  ///     otherwise.
+  bool operator!=(const SourceLocationSpec &rhs) const;
+
+  /// Less than to operator
+  ///
+  /// Tests if this object is less than \a rhs.
+  ///
+  /// \param[in] rhs
+  ///     A const SourceLocationSpec object reference to compare this object
+  ///     to.
+  ///
+  /// \return
+  ///     \b true if this object is less than \a rhs, \b false
+  ///     otherwise.
+  bool operator<(const SourceLocationSpec &rhs) const;
+
+  /// Convert to pointer operator.
+  ///
+  /// This allows code to check a SourceLocationSpec object to see if it
+  /// contains anything valid using code such as:
+  ///
+  /// \code
+  /// SourceLocationSpec location_spec(...);
+  /// if (location_spec)
+  /// { ...
+  /// \endcode
+  ///
+  /// \return
+  ///     A pointer to this object if both the file_spec and the line are valid,
+  ///     nullptr otherwise.
+  explicit operator bool() const;
+
+  /// Logical NOT operator.
+  ///
+  /// This allows code to check a SourceLocationSpec object to see if it is
+  /// invalid using code such as:
+  ///
+  /// \code
+  /// SourceLocationSpec location_spec(...);
+  /// if (!location_spec)
+  /// { ...
+  /// \endcode
+  ///
+  /// \return
+  ///     Returns \b true if the object has an invalid file_spec or line number,
+  ///     \b false otherwise.
+  bool operator!() const;
+
+  /// Clears the object state.
+  ///
+  /// Clear this object by releasing the file_spec object
+  /// and making both line and column value the empty string.
+  void Clear();
+
+  /// Compare two SourceLocationSpec objects.
+  ///
+  /// If \a full is true, then the file_spec, the line and column must match.
+  /// If \a full is false, then only the file_spec and line number for \a lhs
+  /// and \a rhs are compared. This allows a SourceLocationSpec object that have
+  /// no column information to match a  SourceLocationSpec objects that have
+  /// column information with matching file_spec and line component.
+  ///
+  /// \param[in] lhs
+  ///     A const reference to the Left Hand Side object to compare.
+  ///
+  /// \param[in] rhs
+  ///     A const reference to the Right Hand Side object to compare.
+  ///
+  /// \param[in] full
+  ///     If true, then the file_spec, the line and column must match for a
+  ///     compare to return zero (equal to). If false, then only the file_spec
+  ///     and line number for \a lhs and \a rhs are compared, else a full
+  ///     comparison is done.
+  ///
+  /// \return -1 if \a lhs is less than \a rhs, 0 if \a lhs is equal to \a rhs,
+  ///     1 if \a lhs is greater than \a rhs
+  static int Compare(const SourceLocationSpec &lhs,
+                     const SourceLocationSpec &rhs, bool full);
+
+  static bool Equal(const SourceLocationSpec &a, const SourceLocationSpec &b,
+                    bool full);
+
+  /// Match SourceLocationSpec \a pattern against SourceLocationSpec \a
+  /// location. If \a pattern has a column number, then the \a location must
+  /// have the same column number. Otherwise, it should only matches just the
+  /// file_spec and line number. An empty \a pattern matches everything.
+  static bool Match(const SourceLocationSpec &pattern,
+                    const SourceLocationSpec &location);
+
+  /// Dump this object to a Stream.
+  ///
+  /// Dump the object to the supplied stream \a s, starting with the file name,
+  /// then the line number and if available the column number.
+  ///
+  /// \param[in] s
+  ///     The stream to which to dump the object description.
+  void Dump(llvm::raw_ostream &s) const;
+
+  const char *GetCString() const;
+
+  FileSpec GetFileSpec() const { return m_file_spec; }
+
+  uint32_t GetLine() const { return m_line; }
+
+  bool HasColumn() const { return m_column.hasValue() && *m_column != 0; }
+
+  llvm::Optional<uint16_t> GetColumn() const { return m_column; }
+
+protected:
+  FileSpec m_file_spec;
+  uint32_t m_line;
+  llvm::Optional<uint16_t> m_column;
+};
+
+/// Dump a SourceLocationSpec object to a stream
+Stream &operator<<(Stream &s, const SourceLocationSpec &loc);
+} // namespace lldb_private
+
+#endif // LLDB_UTILITY_SOURCELOCATIONSPEC_H

lldb/source/Utility/CMakeLists.txt

add_lldb_library(lldbUtility
   RegisterValue.cpp
   RegularExpression.cpp
   Reproducer.cpp
   ReproducerInstrumentation.cpp
   ReproducerProvider.cpp
   Scalar.cpp
   SelectHelper.cpp
   State.cpp
+  SourceLocationSpec.cpp
   Status.cpp
   Stream.cpp
   StreamCallback.cpp
   StreamString.cpp
   StringExtractor.cpp
   StringExtractorGDBRemote.cpp
   StringLexer.cpp
   StringList.cpp

lldb/source/Utility/SourceLocationSpec.cpp

+//===-- SourceLocationSpec.cpp --------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/SourceLocationSpec.h"
+#include "lldb/Utility/Stream.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SourceLocationSpec::SourceLocationSpec(FileSpec file_spec, uint32_t line)
+    : m_file_spec(file_spec), m_line(line), m_column(llvm::None) {}
+
+SourceLocationSpec::SourceLocationSpec(FileSpec file_spec, uint32_t line,
+                                       uint16_t column)
+    : SourceLocationSpec(file_spec, line) {
+  if (column)
+    m_column = column;
+}
+
+SourceLocationSpec::operator bool() const { return m_file_spec && m_line; }
+
+bool SourceLocationSpec::operator!() const { return !operator bool(); }
+
+bool SourceLocationSpec::operator==(const SourceLocationSpec &rhs) const {
+  return m_file_spec == rhs.GetFileSpec() && m_line == rhs.GetLine() &&
+         m_column == rhs.GetColumn();
+}
+
+bool SourceLocationSpec::operator!=(const SourceLocationSpec &rhs) const {
+  return !(*this == rhs);
+}
+
+bool SourceLocationSpec::operator<(const SourceLocationSpec &rhs) const {
+  return SourceLocationSpec::Compare(*this, rhs, true) < 0;
+}
+
+Stream &lldb_private::operator<<(Stream &s, const SourceLocationSpec &loc) {
+  loc.Dump(s.AsRawOstream());
+  return s;
+}
+
+void SourceLocationSpec::Clear() {
+  m_file_spec.Clear();
+  m_line = 0;
+  m_column = llvm::None;
+}
+
+int SourceLocationSpec::Compare(const SourceLocationSpec &a,
+                                const SourceLocationSpec &b, bool full) {
+
+  if (int result = FileSpec::Compare(a.GetFileSpec(), b.GetFileSpec(), full))
+    return result;
+
+  // If both file_spec matches, compare line numbers.
+  if (a.GetLine() != b.GetLine()) {
+    return (a.GetLine() < b.GetLine()) ? -1 : 1;
+  }
+
+  // If both file_spec and line number match, and \a full comparaison is
+  // enabled, compare column number.
+  if (full && a.GetColumn() != b.GetColumn())
+    return (a.GetColumn() < b.GetColumn()) ? -1 : 1;
+
+  return 0;
+}
+
+bool SourceLocationSpec::Equal(const SourceLocationSpec &a,
+                               const SourceLocationSpec &b, bool full) {
+  return full ? a == b : Match(a, b);
+}
+
+bool SourceLocationSpec::Match(const SourceLocationSpec &pattern,
+                               const SourceLocationSpec &location) {
+  return pattern.GetFileSpec() == location.GetFileSpec() &&
+         pattern.GetLine() == location.GetLine();
+}
+
+void SourceLocationSpec::Dump(llvm::raw_ostream &s) const {
+  GetFileSpec().Dump(s);
+  s << ':' << GetLine();
+
+  if (HasColumn())
+    s << ':' << GetColumn();
+}
+
+const char *SourceLocationSpec::GetCString() const {
+  std::string data;
+  llvm::raw_string_ostream ss(data);
+  Dump(ss);
+
+  return ConstString{ss.str()}.AsCString();
+}

lldb/unittests/Utility/CMakeLists.txt

add_lldb_unittest(UtilityTests
   RangeMapTest.cpp
   RangeTest.cpp
   RegisterValueTest.cpp
   RegularExpressionTest.cpp
   ReproducerInstrumentationTest.cpp
   ReproducerTest.cpp
   ScalarTest.cpp
   SharedClusterTest.cpp
+  SourceLocationSpecTest.cpp
   StateTest.cpp
   StatusTest.cpp
   StreamTeeTest.cpp
   StreamTest.cpp
   StringExtractorGDBRemoteTest.cpp
   StringExtractorTest.cpp
   StringLexerTest.cpp
   StringListTest.cpp
   StructuredDataTest.cpp
   SubsystemRAIITest.cpp
   TildeExpressionResolverTest.cpp
   TimeoutTest.cpp
   TimerTest.cpp
+  UUIDTest.cpp
   UriParserTest.cpp
   UserIDResolverTest.cpp
-  UUIDTest.cpp
   VASprintfTest.cpp
   VMRangeTest.cpp
   XcodeSDKTest.cpp
 
   LINK_LIBS
       lldbUtility
       lldbUtilityHelpers
       LLVMTestingSupport
   LINK_COMPONENTS
     Support
   )
 
 add_unittest_inputs(UtilityTests
   StructuredData-basic.json
   )

lldb/unittests/Utility/SourceLocationSpecTest.cpp

+//===-- SourceLocationSpecTest.cpp
+//--------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/SourceLocationSpec.h"
+
+using namespace lldb_private;
+
+TEST(SourceLocationSpecTest, FileLineColumnComponents) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint16_t column = 4;
+  SourceLocationSpec without_column(fs, line);
+  EXPECT_EQ(fs, without_column.GetFileSpec());
+  EXPECT_EQ(line, without_column.GetLine());
+  EXPECT_FALSE(without_column.HasColumn());
+  EXPECT_EQ(llvm::None, without_column.GetColumn());
+  EXPECT_STREQ("/foo/bar:19", without_column.GetCString());
+
+  SourceLocationSpec with_column(fs, 19, 4);
+  EXPECT_TRUE(with_column.HasColumn());
+  EXPECT_EQ(column, with_column.GetColumn());
+  EXPECT_STREQ("/foo/bar:19:4", with_column.GetCString());
+}
+
+TEST(SourceLocationSpecTest, Equal) {
+  auto Eq = [](SourceLocationSpec a, SourceLocationSpec b, bool full) {
+    return SourceLocationSpec::Equal(a, b, full);
+  };
+
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  EXPECT_TRUE(Eq(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4), true));
+  EXPECT_TRUE(Eq(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4), false));
+
+  EXPECT_FALSE(
+      Eq(SourceLocationSpec(fs, 4), SourceLocationSpec(other_fs, 4), true));
+  EXPECT_FALSE(
+      Eq(SourceLocationSpec(fs, 4), SourceLocationSpec(other_fs, 4), false));
+
+  EXPECT_TRUE(
+      Eq(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_TRUE(
+      Eq(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4, 19), false));
+
+  EXPECT_FALSE(Eq(SourceLocationSpec(fs, 4, 19),
+                  SourceLocationSpec(other_fs, 4, 19), true));
+  EXPECT_FALSE(Eq(SourceLocationSpec(fs, 4, 19),
+                  SourceLocationSpec(other_fs, 4, 19), false));
+
+  EXPECT_FALSE(
+      Eq(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_TRUE(
+      Eq(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4, 19), false));
+
+  EXPECT_FALSE(Eq(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4), true));
+  EXPECT_FALSE(Eq(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4), false));
+
+  EXPECT_FALSE(
+      Eq(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_FALSE(
+      Eq(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4, 19), false));
+
+  EXPECT_FALSE(
+      Eq(SourceLocationSpec(fs, 4, 96), SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_TRUE(
+      Eq(SourceLocationSpec(fs, 4, 96), SourceLocationSpec(fs, 4, 19), false));
+}
+
+TEST(SourceLocationSpecTest, Match) {
+  auto Match = [](SourceLocationSpec a, SourceLocationSpec b) {
+    return SourceLocationSpec::Match(a, b);
+  };
+
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  EXPECT_TRUE(Match(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4)));
+
+  EXPECT_FALSE(
+      Match(SourceLocationSpec(fs, 4), SourceLocationSpec(other_fs, 4)));
+  EXPECT_FALSE(Match(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 19)));
+  EXPECT_FALSE(
+      Match(SourceLocationSpec(fs, 4), SourceLocationSpec(other_fs, 19)));
+
+  EXPECT_TRUE(
+      Match(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4, 19)));
+  EXPECT_TRUE(
+      Match(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4, 96)));
+
+  EXPECT_FALSE(Match(SourceLocationSpec(fs, 4, 19),
+                     SourceLocationSpec(other_fs, 4, 96)));
+  EXPECT_FALSE(
+      Match(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 19, 96)));
+  EXPECT_FALSE(Match(SourceLocationSpec(fs, 4, 19),
+                     SourceLocationSpec(other_fs, 19, 96)));
+
+  EXPECT_TRUE(Match(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4, 19)));
+  EXPECT_TRUE(Match(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4)));
+
+  EXPECT_FALSE(
+      Match(SourceLocationSpec(other_fs, 4), SourceLocationSpec(fs, 4, 19)));
+  EXPECT_FALSE(
+      Match(SourceLocationSpec(other_fs, 4, 19), SourceLocationSpec(fs, 4)));
+}
+
+TEST(SourceLocationSpecTest, Compare) {
+  auto Cmp = [](SourceLocationSpec a, SourceLocationSpec b, bool full) {
+    return SourceLocationSpec::Compare(a, b, full);
+  };
+
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  // Asymetric comparaison
+  EXPECT_EQ(
+      -1, Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_EQ(
+      0, Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4, 19), false));
+  EXPECT_EQ(
+      1, Cmp(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4), true));
+
+  // mutable FS, const Line
+  EXPECT_EQ(-1, Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(other_fs, 4),
+                    true));
+  EXPECT_EQ(-1, Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(other_fs, 4),
+                    false));
+  EXPECT_EQ(
+      1, Cmp(SourceLocationSpec(other_fs, 4), SourceLocationSpec(fs, 4), true));
+  EXPECT_EQ(1, Cmp(SourceLocationSpec(other_fs, 4), SourceLocationSpec(fs, 4),
+                   false));
+
+  // const FS, mutable Line
+  EXPECT_EQ(-1,
+            Cmp(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4), true));
+  EXPECT_EQ(-1,
+            Cmp(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4), false));
+  EXPECT_EQ(0, Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4), true));
+  EXPECT_EQ(0,
+            Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 4), false));
+  EXPECT_EQ(1, Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 0), true));
+  EXPECT_EQ(1,
+            Cmp(SourceLocationSpec(fs, 4), SourceLocationSpec(fs, 0), false));
+
+  // const FS, mutable Line, const Column
+  EXPECT_EQ(
+      -1, Cmp(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_EQ(
+      -1, Cmp(SourceLocationSpec(fs, 0), SourceLocationSpec(fs, 4, 19), false));
+  EXPECT_EQ(
+      1, Cmp(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 0), true));
+  EXPECT_EQ(
+      1, Cmp(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 0), false));
+
+  // mutable FS, const Line, const Column
+  EXPECT_EQ(-1, Cmp(SourceLocationSpec(fs, 4, 19),
+                    SourceLocationSpec(other_fs, 4, 19), true));
+  EXPECT_EQ(-1, Cmp(SourceLocationSpec(fs, 4, 19),
+                    SourceLocationSpec(other_fs, 4, 19), false));
+  EXPECT_EQ(0, Cmp(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4, 19),
+                   true));
+  EXPECT_EQ(0, Cmp(SourceLocationSpec(fs, 4, 19), SourceLocationSpec(fs, 4, 19),
+                   false));
+  EXPECT_EQ(1, Cmp(SourceLocationSpec(other_fs, 4, 19),
+                   SourceLocationSpec(fs, 4, 19), true));
+  EXPECT_EQ(1, Cmp(SourceLocationSpec(other_fs, 4, 19),
+                   SourceLocationSpec(fs, 4, 19), false));
+
+  // const FS, const Line, mutable Column
+  EXPECT_EQ(-1, Cmp(SourceLocationSpec(fs, 4, 19),
+                    SourceLocationSpec(fs, 4, 96), true));
+  EXPECT_EQ(0, Cmp(SourceLocationSpec(fs, 4, 96), SourceLocationSpec(fs, 4, 19),
+                   false));
+  EXPECT_EQ(1, Cmp(SourceLocationSpec(fs, 4, 96), SourceLocationSpec(fs, 4, 19),
+                   true));
+}
+
+TEST(SourceLocationSpecTest, OperatorBool) {
+  SourceLocationSpec invalid(FileSpec(), 0);
+  EXPECT_FALSE(invalid);
+
+  SourceLocationSpec invalid_filespec(FileSpec(), 4);
+  EXPECT_FALSE(invalid_filespec);
+
+  SourceLocationSpec invalid_line(FileSpec("/foo/bar"), 0);
+  EXPECT_FALSE(invalid_line);
+
+  SourceLocationSpec valid_fs_line_no_column(FileSpec("/foo/bar"), 4);
+  EXPECT_TRUE(valid_fs_line_no_column);
+
+  SourceLocationSpec invalid_fs_column(FileSpec(), 4, 0);
+  EXPECT_FALSE(invalid_fs_column);
+
+  SourceLocationSpec invalid_line_column(FileSpec("/foo/bar"), 0, 19);
+  EXPECT_FALSE(invalid_line_column);
+
+  SourceLocationSpec valid_fs_line_zero_column(FileSpec("/foo/bar"), 4, 0);
+  EXPECT_TRUE(valid_fs_line_zero_column);
+
+  SourceLocationSpec valid_fs_line_column(FileSpec("/foo/bar"), 4, 19);
+  EXPECT_TRUE(valid_fs_line_column);
+}