[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 ↗

(On Diff #339246)

90 ↗

(On Diff #339246)

126 ↗

(On Diff #339246)

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

153 ↗

(On Diff #339246)

160 ↗

(On Diff #339246)

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 ↗

(On Diff #339246)

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 ↗

(On Diff #339246)

Are there situations where the line is optional too?

lldb/unittests/Utility/SourceLocationSpecTest.cpp

1–2 ↗

(On Diff #339246)

[mib]

lldb/include/lldb/Utility/SourceLocationSpec.h

184 ↗

(On Diff #339246)

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 ↗

(On Diff #339246)

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 ↗

(On Diff #339246)

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

13–15 ↗

(On Diff #339867)

108–138 ↗

(On Diff #339867)

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

183 ↗

(On Diff #339867)

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.

195–196 ↗

(On Diff #339867)

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

lldb/source/Utility/SourceLocationSpec.cpp

59–61 ↗

(On Diff #339867)

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

lldb/unittests/Utility/SourceLocationSpecTest.cpp

96 ↗

(On Diff #339867)

136–143 ↗

(On Diff #339867)

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

59–61 ↗

(On Diff #339867)

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

41 ↗

(On Diff #341738)

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

[JDevlieghere]

lldb/source/Core/SourceLocationSpec.cpp

49–62

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

73–78

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

58

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

66–72

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/Core/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 "lldb/Core/Declaration.h"
+#include "lldb/lldb-defines.h"
+#include "llvm/ADT/Optional.h"
+
+#include <string>
+
+namespace lldb_private {
+
+/// \class SourceLocationSpec SourceLocationSpec.h
+/// "lldb/Core/SourceLocationSpec.h" A source location specifier class.
+///
+/// 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.
+class SourceLocationSpec {
+public:
+  /// 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.
+  ///
+  ///  \param[in] check_inlines
+  ///     Whether to look for a match in inlined declaration.
+  ///
+  ///  \param[in] exact_match
+  ///     Whether to look for an exact match.
+  ///
+  explicit SourceLocationSpec(FileSpec file_spec, uint32_t line,
+                              llvm::Optional<uint16_t> column = llvm::None,
+                              bool check_inlines = false,
+                              bool exact_match = false);
+
+  SourceLocationSpec() = delete;
+
+  /// 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(...);
+  /// 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;
+
+  /// 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;
+
+  /// 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);
+
+  static bool Equal(const SourceLocationSpec &lhs,
+                    const SourceLocationSpec &rhs, bool full);
+
+  /// 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(Stream &s) const;
+
+  std::string GetString() const;
+
+  FileSpec GetFileSpec() const { return m_declaration.GetFile(); }
+
+  llvm::Optional<uint32_t> GetLine() const;
+
+  llvm::Optional<uint16_t> GetColumn() const;
+
+  bool GetCheckInlines() const { return m_check_inlines; }
+
+  bool GetExactMatch() const { return m_exact_match; }
+
+protected:
+  Declaration m_declaration;
+  /// Tells if the resolver should look in inlined declaration.
+  bool m_check_inlines;
+  /// Tells if the resolver should look for an exact match.
+  bool m_exact_match;
+};
+
+/// Dump a SourceLocationSpec object to a stream
+Stream &operator<<(Stream &s, const SourceLocationSpec &loc);
+} // namespace lldb_private
+
+#endif // LLDB_UTILITY_SOURCELOCATIONSPEC_H

lldb/source/Core/CMakeLists.txt

add_lldb_library(lldbCore
   ModuleChild.cpp
   ModuleList.cpp
   Opcode.cpp
   PluginManager.cpp
   Progress.cpp
   RichManglingContext.cpp
   SearchFilter.cpp
   Section.cpp
+  SourceLocationSpec.cpp
   SourceManager.cpp
   StreamAsynchronousIO.cpp
   StreamFile.cpp
   UserSettingsController.cpp
   Value.cpp
   ValueObject.cpp
   ValueObjectCast.cpp
   ValueObjectChild.cpp

lldb/source/Core/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/Core/SourceLocationSpec.h"
+#include "lldb/Utility/StreamString.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SourceLocationSpec::SourceLocationSpec(FileSpec file_spec, uint32_t line,
+                                       llvm::Optional<uint16_t> column,
+                                       bool check_inlines, bool exact_match)
+    : m_declaration(file_spec, line,
+                    column.getValueOr(LLDB_INVALID_COLUMN_NUMBER)),
+      m_check_inlines(check_inlines), m_exact_match(exact_match) {}
+
+SourceLocationSpec::operator bool() const { return m_declaration.IsValid(); }
+
+bool SourceLocationSpec::operator!() const { return !operator bool(); }
+
+bool SourceLocationSpec::operator==(const SourceLocationSpec &rhs) const {
+  return m_declaration == rhs.m_declaration &&
+         m_check_inlines == rhs.GetCheckInlines() &&
+         m_exact_match == rhs.GetExactMatch();
+}
+
+bool SourceLocationSpec::operator!=(const SourceLocationSpec &rhs) const {
+  return !(*this == rhs);
+}
+
+bool SourceLocationSpec::operator<(const SourceLocationSpec &rhs) const {
+  return SourceLocationSpec::Compare(*this, rhs) < 0;
+}
+
+Stream &lldb_private::operator<<(Stream &s, const SourceLocationSpec &loc) {
+  loc.Dump(s);
+  return s;
+}
+
+int SourceLocationSpec::Compare(const SourceLocationSpec &lhs,
+                                const SourceLocationSpec &rhs) {
+  return Declaration::Compare(lhs.m_declaration, rhs.m_declaration);
+}
+
+bool SourceLocationSpec::Equal(const SourceLocationSpec &lhs,
+                               const SourceLocationSpec &rhs, bool full) {
+  return full ? lhs == rhs
+              : (lhs.GetFileSpec() == rhs.GetFileSpec() &&
+                 lhs.GetLine() == rhs.GetLine());
+}
+
+void SourceLocationSpec::Dump(Stream &s) const {
+  s << "check inlines = " << llvm::toStringRef(m_check_inlines);
+  s << ", exact match = " << llvm::toStringRef(m_exact_match);
+  m_declaration.Dump(&s, true);
+}
+
+std::string SourceLocationSpec::GetString() const {
+  StreamString ss;
+  Dump(ss);
+  return ss.GetString().str();
+}
+
+llvm::Optional<uint32_t> SourceLocationSpec::GetLine() const {
+  uint32_t line = m_declaration.GetLine();
+  if (line == 0 || line == LLDB_INVALID_LINE_NUMBER)
+    return llvm::None;
+  return line;
+}
+
+llvm::Optional<uint16_t> SourceLocationSpec::GetColumn() const {
+  uint16_t column = m_declaration.GetColumn();
+  if (column == LLDB_INVALID_COLUMN_NUMBER)
+    return llvm::None;
+  return column;
+}

lldb/unittests/Core/CMakeLists.txt

 add_lldb_unittest(LLDBCoreTests
   CommunicationTest.cpp
   DumpDataExtractorTest.cpp
   FormatEntityTest.cpp
   MangledTest.cpp
   ModuleSpecTest.cpp
   RichManglingContextTest.cpp
+  SourceLocationSpecTest.cpp
   SourceManagerTest.cpp
   StreamCallbackTest.cpp
   UniqueCStringMapTest.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbSymbol

lldb/unittests/Core/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/Core/SourceLocationSpec.h"
+#include "lldb/Utility/LLDBAssert.h"
+
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+
+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);
+}
+
+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, LLDB_INVALID_COLUMN_NUMBER, false,
+                                    true);
+  EXPECT_TRUE(without_column);
+  EXPECT_EQ(fs, without_column.GetFileSpec());
+  EXPECT_EQ(line, without_column.GetLine().getValueOr(0));
+  EXPECT_EQ(llvm::None, without_column.GetColumn());
+  EXPECT_FALSE(without_column.GetCheckInlines());
+  EXPECT_TRUE(without_column.GetExactMatch());
+  EXPECT_STREQ("check inlines = false, exact match = true, decl = /foo/bar:19",
+               without_column.GetString().c_str());
+
+  SourceLocationSpec with_column(fs, line, column, true, false);
+  EXPECT_TRUE(with_column);
+  EXPECT_EQ(column, *with_column.GetColumn());
+  EXPECT_TRUE(with_column.GetCheckInlines());
+  EXPECT_FALSE(with_column.GetExactMatch());
+  EXPECT_STREQ(
+      "check inlines = true, exact match = false, decl = /foo/bar:19:4",
+      with_column.GetString().c_str());
+}
+
+static SourceLocationSpec Create(bool check_inlines, bool exact_match,
+                                 FileSpec fs, uint32_t line,
+                                 uint16_t column = LLDB_INVALID_COLUMN_NUMBER) {
+  return SourceLocationSpec(fs, line, column, check_inlines, exact_match);
+}
+
+TEST(SourceLocationSpecTest, Equal) {
+  auto Equal = [](SourceLocationSpec lhs, SourceLocationSpec rhs, bool full) {
+    return SourceLocationSpec::Equal(lhs, rhs, full);
+  };
+
+  const FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  // mutating FileSpec + const Inlined, ExactMatch, Line
+  EXPECT_TRUE(
+      Equal(Create(false, false, fs, 4), Create(false, false, fs, 4), true));
+  EXPECT_TRUE(
+      Equal(Create(true, true, fs, 4), Create(true, true, fs, 4), false));
+  EXPECT_FALSE(Equal(Create(false, false, fs, 4),
+                     Create(false, false, other_fs, 4), true));
+  EXPECT_FALSE(
+      Equal(Create(true, true, fs, 4), Create(true, true, other_fs, 4), false));
+
+  // Mutating FileSpec + const Inlined, ExactMatch, Line, Column
+  EXPECT_TRUE(Equal(Create(false, false, fs, 4, 19),
+                    Create(false, false, fs, 4, 19), true));
+  EXPECT_TRUE(Equal(Create(true, true, fs, 4, 19),
+                    Create(true, true, fs, 4, 19), false));
+  EXPECT_FALSE(Equal(Create(false, false, fs, 4, 19),
+                     Create(false, false, other_fs, 4, 19), true));
+  EXPECT_FALSE(Equal(Create(true, true, fs, 4, 19),
+                     Create(true, true, other_fs, 4, 19), false));
+
+  // Asymetric match
+  EXPECT_FALSE(
+      Equal(Create(true, true, fs, 4), Create(true, true, fs, 4, 19), true));
+  EXPECT_TRUE(Equal(Create(false, false, fs, 4),
+                    Create(false, false, fs, 4, 19), false));
+
+  // Mutating Inlined, ExactMatch
+  EXPECT_FALSE(
+      Equal(Create(true, false, fs, 4), Create(false, true, fs, 4), true));
+  EXPECT_TRUE(
+      Equal(Create(false, true, fs, 4), Create(true, false, fs, 4), false));
+
+  // Mutating Column
+  EXPECT_FALSE(Equal(Create(true, true, fs, 4, 96),
+                     Create(true, true, fs, 4, 19), true));
+  EXPECT_TRUE(Equal(Create(false, false, fs, 4, 96),
+                    Create(false, false, fs, 4, 19), false));
+}
+
+TEST(SourceLocationSpecTest, Compare) {
+  auto Cmp = [](SourceLocationSpec a, SourceLocationSpec b) {
+    return SourceLocationSpec::Compare(a, b);
+  };
+
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  // Asymetric comparaison
+  EXPECT_EQ(-1, Cmp(Create(true, true, fs, 4), Create(true, true, fs, 4, 19)));
+  EXPECT_EQ(-1,
+            Cmp(Create(false, false, fs, 4), Create(false, false, fs, 4, 19)));
+  EXPECT_EQ(1, Cmp(Create(true, true, fs, 4, 19), Create(true, true, fs, 4)));
+
+  // Mutating FS, const Line
+  EXPECT_EQ(
+      -1, Cmp(Create(false, false, fs, 4), Create(false, false, other_fs, 4)));
+  EXPECT_EQ(-1,
+            Cmp(Create(true, true, fs, 4), Create(true, true, other_fs, 4)));
+  EXPECT_EQ(1,
+            Cmp(Create(false, true, other_fs, 4), Create(false, true, fs, 4)));
+  EXPECT_EQ(1,
+            Cmp(Create(true, false, other_fs, 4), Create(true, false, fs, 4)));
+
+  // Const FS, mutating Line
+  EXPECT_EQ(-1, Cmp(Create(false, false, fs, 1), Create(false, false, fs, 4)));
+  EXPECT_EQ(-1, Cmp(Create(true, true, fs, 1), Create(true, true, fs, 4)));
+  EXPECT_EQ(0, Cmp(Create(false, true, fs, 4), Create(false, true, fs, 4)));
+  EXPECT_EQ(0, Cmp(Create(true, false, fs, 4), Create(true, false, fs, 4)));
+  EXPECT_EQ(1, Cmp(Create(false, false, fs, 4), Create(false, false, fs, 1)));
+  EXPECT_EQ(1, Cmp(Create(true, true, fs, 4), Create(true, true, fs, 1)));
+
+  // Const FS, mutating Line, const Column
+  EXPECT_EQ(-1,
+            Cmp(Create(false, true, fs, 1), Create(false, true, fs, 4, 19)));
+  EXPECT_EQ(-1, Cmp(Create(true, true, fs, 1), Create(true, true, fs, 4, 19)));
+  EXPECT_EQ(1, Cmp(Create(true, false, fs, 4, 19), Create(true, false, fs, 1)));
+  EXPECT_EQ(1, Cmp(Create(true, false, fs, 4, 19), Create(true, false, fs, 1)));
+
+  // Mutating FS, const Line, const Column
+  EXPECT_EQ(-1, Cmp(Create(false, false, fs, 4, 19),
+                    Create(false, false, other_fs, 4, 19)));
+  EXPECT_EQ(-1, Cmp(Create(true, true, fs, 4, 19),
+                    Create(true, true, other_fs, 4, 19)));
+  EXPECT_EQ(
+      0, Cmp(Create(false, false, fs, 4, 19), Create(false, false, fs, 4, 19)));
+  EXPECT_EQ(0,
+            Cmp(Create(true, true, fs, 4, 19), Create(true, true, fs, 4, 19)));
+  EXPECT_EQ(1, Cmp(Create(false, true, other_fs, 4, 19),
+                   Create(false, true, fs, 4, 19)));
+  EXPECT_EQ(1, Cmp(Create(true, false, other_fs, 4, 19),
+                   Create(true, false, fs, 4, 19)));
+
+  // Const FS, const Line, mutating Column
+  EXPECT_EQ(-1, Cmp(Create(false, false, fs, 4, 19),
+                    Create(false, false, fs, 4, 96)));
+  EXPECT_EQ(1,
+            Cmp(Create(true, true, fs, 4, 96), Create(true, true, fs, 4, 19)));
+  EXPECT_EQ(
+      1, Cmp(Create(false, true, fs, 4, 96), Create(false, true, fs, 4, 19)));
+}