From 61a4d3d45c91cb19a114796846e5af014f59a6b6 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 15 Jul 2024 20:08:41 +0200
Subject: [PATCH] getSnippetUpTo: Return optional

This makes it possible to certain discern failures from empty
snippets, which I think is an ok review comment.

Maybe it should do so for swapped column indexes too, but I'm not
sure.

I don't think it matters in the grand scheme. We don't even have
a real use case for `nullopt` now anyway.

Since we don't have a use case, I'm not applying this logic to
higher level functions yet.
---
 src/libexpr/nixexpr.cc         | 2 +-
 src/libutil/position.cc        | 6 +++---
 src/libutil/position.hh        | 2 +-
 tests/unit/libutil/position.cc | 8 +++++---
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
index d54f0b684..93c8bdef6 100644
--- a/src/libexpr/nixexpr.cc
+++ b/src/libexpr/nixexpr.cc
@@ -646,7 +646,7 @@ size_t SymbolTable::totalSize() const
 std::string DocComment::getInnerText(const PosTable & positions) const {
     auto beginPos = positions[begin];
     auto endPos = positions[end];
-    auto docCommentStr = beginPos.getSnippetUpTo(endPos);
+    auto docCommentStr = beginPos.getSnippetUpTo(endPos).value_or("");
 
     // Strip "/**" and "*/"
     constexpr size_t prefixLen = 3;
diff --git a/src/libutil/position.cc b/src/libutil/position.cc
index 508816d85..3289dbe8b 100644
--- a/src/libutil/position.cc
+++ b/src/libutil/position.cc
@@ -110,11 +110,11 @@ void Pos::LinesIterator::bump(bool atFirst)
     input.remove_prefix(eol);
 }
 
-std::string Pos::getSnippetUpTo(const Pos & end) const {
+std::optional<std::string> Pos::getSnippetUpTo(const Pos & end) const {
     assert(this->origin == end.origin);
 
     if (end.line < this->line)
-        return "";
+        return std::nullopt;
 
     if (auto source = getSource()) {
 
@@ -152,7 +152,7 @@ std::string Pos::getSnippetUpTo(const Pos & end) const {
         }
         return result;
     }
-    return "";
+    return std::nullopt;
 }
 
 
diff --git a/src/libutil/position.hh b/src/libutil/position.hh
index aba263fdf..25217069c 100644
--- a/src/libutil/position.hh
+++ b/src/libutil/position.hh
@@ -64,7 +64,7 @@ struct Pos
     bool operator==(const Pos & rhs) const = default;
     auto operator<=>(const Pos & rhs) const = default;
 
-    std::string getSnippetUpTo(const Pos & end) const;
+    std::optional<std::string> getSnippetUpTo(const Pos & end) const;
 
     /**
      * Get the SourcePath, if the source was loaded from a file.
diff --git a/tests/unit/libutil/position.cc b/tests/unit/libutil/position.cc
index d38d2c538..484ecc247 100644
--- a/tests/unit/libutil/position.cc
+++ b/tests/unit/libutil/position.cc
@@ -25,7 +25,7 @@ TEST(Position, getSnippetUpTo_1)
         ASSERT_EQ(start.getSnippetUpTo(start), "");
         ASSERT_EQ(start.getSnippetUpTo(end), "x");
         ASSERT_EQ(end.getSnippetUpTo(end), "");
-        ASSERT_EQ(end.getSnippetUpTo(start), "");
+        ASSERT_EQ(end.getSnippetUpTo(start), std::nullopt);
     }
     {
         // NOTE: line and column are actually 1-based indexes
@@ -37,7 +37,7 @@ TEST(Position, getSnippetUpTo_1)
         ASSERT_EQ(start.getSnippetUpTo(end), "");
 
         ASSERT_EQ(end.getSnippetUpTo(end), "");
-        ASSERT_EQ(end.getSnippetUpTo(start), "");
+        ASSERT_EQ(end.getSnippetUpTo(start), std::nullopt);
     }
     {
         Pos start(1, 1, o);
@@ -53,7 +53,7 @@ TEST(Position, getSnippetUpTo_1)
         ASSERT_EQ(start.getSnippetUpTo(start), "");
         ASSERT_EQ(start.getSnippetUpTo(end), "x");
         ASSERT_EQ(end.getSnippetUpTo(end), "");
-        ASSERT_EQ(end.getSnippetUpTo(start), "");
+        ASSERT_EQ(end.getSnippetUpTo(start), std::nullopt);
     }
 }
 TEST(Position, getSnippetUpTo_2)
@@ -65,6 +65,8 @@ TEST(Position, getSnippetUpTo_2)
         ASSERT_EQ(start.getSnippetUpTo(start), "");
         ASSERT_EQ(start.getSnippetUpTo(end), "a");
         ASSERT_EQ(end.getSnippetUpTo(end), "");
+
+        // nullopt? I feel like changing the column handling would just make it more fragile
         ASSERT_EQ(end.getSnippetUpTo(start), "");
     }
     {