Remove Buffer::Rows() and the Line wrapper

The row cache rebuilt a string for every line of the file after each edit
and had one production caller left (filetype detection in the ImGui
frontend, which needs only the first line). Removed with it: rows_,
nrows_, the dirty flag set on every edit, ensure_rows_cache and a mutex
guarding only that cache. Tests read lines with GetLineString, Nrows or,
where they compare every line, a test-only LinesForTests().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
Claude
2026-09-24 12:19:59 -07:00
committed by Kyle Isom
parent 7dae9cc305
commit 29d0ee8244
16 changed files with 262 additions and 496 deletions
+11 -44
View File
@@ -354,12 +354,9 @@ Buffer::Buffer(const Buffer &other)
{
curx_ = other.curx_;
cury_ = other.cury_;
nrows_ = other.nrows_;
rowoffs_ = other.rowoffs_;
coloffs_ = other.coloffs_;
rows_ = other.rows_;
content_ = other.content_;
rows_cache_dirty_ = other.rows_cache_dirty_;
filename_ = other.filename_;
is_file_backed_ = other.is_file_backed_;
is_virtual_ = other.is_virtual_;
@@ -407,12 +404,9 @@ Buffer::operator=(const Buffer &other)
return *this;
curx_ = other.curx_;
cury_ = other.cury_;
nrows_ = other.nrows_;
rowoffs_ = other.rowoffs_;
coloffs_ = other.coloffs_;
rows_ = other.rows_;
content_ = other.content_;
rows_cache_dirty_ = other.rows_cache_dirty_;
filename_ = other.filename_;
is_file_backed_ = other.is_file_backed_;
is_virtual_ = other.is_virtual_;
@@ -455,10 +449,8 @@ Buffer::operator=(const Buffer &other)
Buffer::Buffer(Buffer &&other) noexcept
: curx_(other.curx_),
cury_(other.cury_),
nrows_(other.nrows_),
rowoffs_(other.rowoffs_),
coloffs_(other.coloffs_),
rows_(std::move(other.rows_)),
filename_(std::move(other.filename_)),
is_file_backed_(other.is_file_backed_),
is_virtual_(other.is_virtual_),
@@ -483,7 +475,6 @@ Buffer::Buffer(Buffer &&other) noexcept
filetype_ = std::move(other.filetype_);
highlighter_ = std::move(other.highlighter_);
content_ = std::move(other.content_);
rows_cache_dirty_ = other.rows_cache_dirty_;
on_disk_identity_ = other.on_disk_identity_;
// Non-owning: the recorder object itself is owned by SwapManager and outlives
// this move. The caller (Editor) is responsible for calling SwapManager::Rehome()
@@ -506,10 +497,8 @@ Buffer::operator=(Buffer &&other) noexcept
curx_ = other.curx_;
cury_ = other.cury_;
nrows_ = other.nrows_;
rowoffs_ = other.rowoffs_;
coloffs_ = other.coloffs_;
rows_ = std::move(other.rows_);
filename_ = std::move(other.filename_);
is_file_backed_ = other.is_file_backed_;
is_virtual_ = other.is_virtual_;
@@ -534,7 +523,6 @@ Buffer::operator=(Buffer &&other) noexcept
filetype_ = std::move(other.filetype_);
highlighter_ = std::move(other.highlighter_);
content_ = std::move(other.content_);
rows_cache_dirty_ = other.rows_cache_dirty_;
on_disk_identity_ = other.on_disk_identity_;
// Non-owning: the recorder object itself is owned by SwapManager and outlives
// this move. The caller (Editor) is responsible for calling SwapManager::Rehome()
@@ -588,8 +576,6 @@ Buffer::OpenFromFile(const std::string &path, std::string &err)
return false;
}
if (!exists) {
rows_.clear();
nrows_ = 0;
filename_ = norm;
is_file_backed_ = false;
is_virtual_ = false;
@@ -603,7 +589,6 @@ Buffer::OpenFromFile(const std::string &path, std::string &err)
// Empty PieceTable
content_.Clear();
rows_cache_dirty_ = true;
// History recorded against the previous content no longer applies.
if (undo_sys_)
undo_sys_->clear();
@@ -662,9 +647,7 @@ Buffer::OpenFromFile(const std::string &path, std::string &err)
// copy runs out of memory, the buffer keeps its old text (clearing first
// left an empty buffer that a later save wrote over the file).
// The bytes become the piece table's original storage, uncopied.
content_.AdoptOriginal(std::move(data));
rows_cache_dirty_ = true;
nrows_ = 0; // not used under PieceTable
content_.AdoptOriginal(std::move(data)); // not used under PieceTable
filename_ = norm;
is_file_backed_ = true;
is_virtual_ = false;
@@ -778,7 +761,6 @@ Buffer::insert_text(int row, int col, std::string_view text)
static_cast<std::size_t>(col));
if (!text.empty()) {
content_.Insert(off, text.data(), text.size());
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_)
swap_rec_->OnInsert(row, col, text);
@@ -807,24 +789,6 @@ Buffer::GetLineView(std::size_t row) const
}
void
Buffer::ensure_rows_cache() const
{
std::lock_guard<std::mutex> lock(buffer_mutex_);
if (!rows_cache_dirty_)
return;
rows_.clear();
const std::size_t lc = content_.LineCount();
rows_.reserve(lc);
for (std::size_t i = 0; i < lc; ++i) {
rows_.emplace_back(content_.GetLine(i));
}
// Keep nrows_ in sync for any legacy code that still reads it
const_cast<Buffer *>(this)->nrows_ = rows_.size();
rows_cache_dirty_ = false;
}
std::size_t
Buffer::content_LineCount_() const
{
@@ -838,6 +802,16 @@ Buffer::BytesForTests() const
{
return Bytes();
}
std::vector<std::string>
Buffer::LinesForTests() const
{
std::vector<std::string> out;
for (std::size_t i = 0; i < Nrows(); ++i)
out.push_back(GetLineString(i));
return out;
}
#endif
@@ -860,7 +834,6 @@ Buffer::delete_text(int row, int col, std::size_t len)
if (actual == 0)
return;
content_.Delete(start, actual);
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_)
swap_rec_->OnDelete(row, col, actual);
@@ -879,7 +852,6 @@ Buffer::split_line(int row, const int col)
static_cast<std::size_t>(c));
const char nl = '\n';
content_.Insert(off, &nl, 1);
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_)
swap_rec_->OnInsert(row, c, std::string_view("\n", 1));
@@ -899,7 +871,6 @@ Buffer::join_lines(int row)
std::size_t end_of_line = content_.LineColToByteOffset(r, std::numeric_limits<std::size_t>::max());
// end_of_line now equals line end (clamped before newline). The newline should be exactly at this position.
content_.Delete(end_of_line, 1);
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_)
swap_rec_->OnDelete(row, col, 1);
@@ -916,7 +887,6 @@ Buffer::insert_row(int row, const std::string_view text)
content_.Insert(off, text.data(), text.size());
const char nl = '\n';
content_.Insert(off + text.size(), &nl, 1);
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_) {
// One record: the first of two could trigger a journal checkpoint,
@@ -948,7 +918,6 @@ Buffer::delete_row(int row)
if (actual == 0)
return;
content_.Delete(start, actual);
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_)
swap_rec_->OnDelete(row, 0, actual);
@@ -961,7 +930,6 @@ Buffer::replace_all_bytes(const std::string_view bytes)
content_.Clear();
if (!bytes.empty())
content_.Append(bytes.data(), bytes.size());
rows_cache_dirty_ = true;
if (undo_sys_)
undo_sys_->clear();
MarkContentChanged();
@@ -978,7 +946,6 @@ Buffer::insert_spans(int row, int col, const std::vector<TextSpan> &spans)
const std::size_t off = content_.LineColToByteOffset(static_cast<std::size_t>(row),
static_cast<std::size_t>(col));
content_.InsertSpans(off, spans);
rows_cache_dirty_ = true;
edited_at_(row);
if (swap_rec_) {
// The journal needs the bytes as one record (see insert_row).
+7 -178
View File
@@ -22,13 +22,8 @@
* - Buffers can be file-backed (associated with a path) or scratch (unnamed)
* - File identity tracking detects external modifications
*
* 3. Legacy Line wrapper:
* - Buffer::Line provides a string-like interface for legacy command code
* - New code should prefer direct PieceTable operations
* - See DEVELOPER_GUIDE.md for migration guidance
*
* 4. Content access:
* - Rows(): Materialized line cache (legacy, being phased out)
* 3. Content access:
* - Nrows(), GetLineString(): per-line access
* - GetLineView(): line access via string_view (zero-copy within a piece)
* - Direct PieceTable access for new editing operations
*/
@@ -136,169 +131,7 @@ public:
}
// Line wrapper used by legacy command paths.
// Keep this lightweight: store materialized bytes only for that line.
class Line {
public:
Line() = default;
explicit Line(const char *s)
{
assign_from(s ? std::string(s) : std::string());
}
explicit Line(const std::string &s)
{
assign_from(s);
}
Line(const Line &other) = default;
Line &operator=(const Line &other) = default;
Line(Line &&other) noexcept = default;
Line &operator=(Line &&other) noexcept = default;
// capacity helpers
void Clear()
{
s_.clear();
}
// size/access
[[nodiscard]] std::size_t size() const
{
return s_.size();
}
[[nodiscard]] bool empty() const
{
return s_.empty();
}
// read-only raw view
[[nodiscard]] const char *Data() const
{
return s_.data();
}
[[nodiscard]] std::size_t Size() const
{
return s_.size();
}
// element access (read-only)
[[nodiscard]] char operator[](std::size_t i) const
{
return (i < s_.size()) ? s_[i] : '\0';
}
// conversions
explicit operator std::string() const
{
return s_;
}
// string-like API used by command/renderer layers (implemented via materialization for now)
[[nodiscard]] std::string substr(std::size_t pos) const
{
return pos < s_.size() ? s_.substr(pos) : std::string();
}
[[nodiscard]] std::string substr(std::size_t pos, std::size_t len) const
{
return pos < s_.size() ? s_.substr(pos, len) : std::string();
}
// minimal find() to support search within a line
[[nodiscard]] std::size_t find(const std::string &needle, const std::size_t pos = 0) const
{
return s_.find(needle, pos);
}
void erase(std::size_t pos)
{
if (pos < s_.size())
s_.erase(pos);
}
void erase(std::size_t pos, std::size_t len)
{
if (pos < s_.size())
s_.erase(pos, len);
}
void insert(std::size_t pos, const std::string &seg)
{
if (pos > s_.size())
pos = s_.size();
s_.insert(pos, seg);
}
Line &operator+=(const Line &other)
{
s_ += other.s_;
return *this;
}
Line &operator+=(const std::string &s)
{
s_ += s;
return *this;
}
Line &operator=(const std::string &s)
{
assign_from(s);
return *this;
}
private:
void assign_from(const std::string &s)
{
s_ = s;
}
std::string s_;
};
[[nodiscard]] const std::vector<Line> &Rows() const
{
ensure_rows_cache();
return rows_;
}
[[nodiscard]] std::vector<Line> &Rows()
{
ensure_rows_cache();
return rows_;
}
// Lightweight, lazy per-line accessors that avoid materializing all rows.
// Prefer these over Rows() in hot paths to reduce memory overhead on large files.
// A line's text without its trailing newline.
[[nodiscard]] std::string GetLineString(std::size_t row) const
{
return content_.GetLine(row);
@@ -722,6 +555,10 @@ public:
#if defined(KTE_TESTS)
// Test-only: return the raw buffer bytes (including newlines) as a string.
// Every line without its newline (tests only).
[[nodiscard]] std::vector<std::string> LinesForTests() const;
[[nodiscard]] std::string BytesForTests() const;
#endif
@@ -742,15 +579,9 @@ private:
// State mirroring original C struct (without undo_tree)
std::size_t curx_ = 0, cury_ = 0; // cursor position in characters
std::size_t nrows_ = 0; // number of rows
std::size_t rowoffs_ = 0, coloffs_ = 0; // viewport offsets
mutable std::vector<Line> rows_; // materialized cache of rows (without trailing newlines)
// PieceTable is the source of truth.
PieceTable content_{};
mutable bool rows_cache_dirty_ = true; // invalidate on edits / I/O
// Helper to rebuild rows_ from content_
void ensure_rows_cache() const;
// Helper to query content_.LineCount() while keeping header minimal
std::size_t content_LineCount_() const;
@@ -798,6 +629,4 @@ private:
std::unique_ptr<kte::HighlighterEngine> highlighter_;
// Non-owning pointer to swap recorder managed by Editor/SwapManager
kte::SwapRecorder *swap_rec_ = nullptr;
mutable std::mutex buffer_mutex_;
};
+1 -4
View File
@@ -56,10 +56,7 @@ apply_syntax_to_buffer(Buffer *b, const GUIConfig &cfg)
b->EnsureHighlighter();
if (auto *eng = b->Highlighter()) {
if (!eng->HasHighlighter()) {
std::string first_line;
const auto &rows = b->Rows();
if (!rows.empty())
first_line = static_cast<std::string>(rows[0]);
const std::string first_line = b->Nrows() > 0 ? b->GetLineString(0) : std::string();
std::string ft = kte::HighlighterRegistry::DetectForPath(
b->Filename(), first_line);
if (!ft.empty()) {
+1 -1
View File
@@ -94,7 +94,7 @@ public:
[[nodiscard]] std::string
Text() const
{
const auto &rows = Buf().Rows();
const auto &rows = Buf().LinesForTests();
std::string out;
for (std::size_t i = 0; i < rows.size(); ++i) {
out += static_cast<std::string>(rows[i]);
+9 -36
View File
@@ -7,7 +7,7 @@
* Benchmarks cover:
* - PieceTable operations (insert, delete, GetLine, GetLineRange)
* - Buffer operations (Nrows, GetLineString, GetLineView)
* - Iteration patterns (comparing old Rows() vs new GetLineString/GetLineView)
* - Iteration patterns (GetLineString vs GetLineView)
* - Syntax highlighting on large files
*
* Each benchmark reports execution time in milliseconds.
@@ -240,26 +240,6 @@ TEST (Benchmark_Buffer_GetLineView_Sequential)
}
TEST (Benchmark_Buffer_Rows_Materialization)
{
std::cout << "\n=== Buffer Rows() Materialization Benchmark ===\n";
Buffer buf;
std::string data = generate_large_file(10000, 80);
buf.insert_text(0, 0, data);
std::size_t total_chars = 0;
{
BenchmarkTimer timer("Rows() materialization + iteration on 10K lines");
const auto &rows = buf.Rows();
for (std::size_t i = 0; i < rows.size(); ++i) {
total_chars += rows[i].size();
}
}
EXPECT_TRUE(total_chars > 0);
}
TEST (Benchmark_Buffer_Iteration_Comparison)
{
std::cout << "\n=== Buffer Iteration Pattern Comparison ===\n";
@@ -267,16 +247,8 @@ TEST (Benchmark_Buffer_Iteration_Comparison)
std::string data = generate_large_file(5000, 80);
buf.insert_text(0, 0, data);
std::size_t sum1 = 0, sum2 = 0, sum3 = 0;
// Pattern 1: Old style with Rows()
{
BenchmarkTimer timer("Pattern 1: Rows() + iteration");
const auto &rows = buf.Rows();
for (std::size_t i = 0; i < rows.size(); ++i) {
sum1 += rows[i].size();
}
}
std::size_t sum2 = 0, sum3 = 0;
std::size_t newlines = 0;
// Pattern 2: New style with GetLineString
{
@@ -290,14 +262,15 @@ TEST (Benchmark_Buffer_Iteration_Comparison)
{
BenchmarkTimer timer("Pattern 3: Nrows() + GetLineView (zero-copy)");
for (std::size_t i = 0; i < buf.Nrows(); ++i) {
sum3 += buf.GetLineView(i).size();
const auto v = buf.GetLineView(i);
sum3 += v.size();
newlines += !v.empty() && v.back() == '\n';
}
}
// sum1 and sum2 should match (both strip newlines)
ASSERT_EQ(sum1, sum2);
// sum3 includes newlines, so it will be larger
EXPECT_TRUE(sum3 > sum2);
// GetLineView includes the newlines GetLineString strips.
ASSERT_EQ(sum3, sum2 + newlines);
EXPECT_TRUE(newlines > 0);
}
+1 -1
View File
@@ -56,7 +56,7 @@ static void
check_buffer_matches_model(const Buffer &b, const std::string &model)
{
auto expected_lines = split_lines_preserve_trailing_empty(model);
const auto &rows = b.Rows();
const auto &rows = b.LinesForTests();
ASSERT_EQ(rows.size(), expected_lines.size());
ASSERT_EQ(b.Nrows(), rows.size());
+1 -1
View File
@@ -51,7 +51,7 @@ read_file_bytes(const std::string &path)
static std::string
buffer_bytes_via_views(const Buffer &b)
{
const auto &rows = b.Rows();
const auto &rows = b.LinesForTests();
std::string out;
for (std::size_t i = 0; i < rows.size(); i++) {
auto v = b.GetLineView(i);
+11 -11
View File
@@ -2,13 +2,13 @@
* test_migration_coverage.cc - Edge case tests for Buffer::Line migration
*
* This file provides comprehensive test coverage for the migration from
* Buffer::Rows() to direct PieceTable operations using Nrows(), GetLineString(),
* the removed Buffer::Rows() to direct PieceTable operations using Nrows(), GetLineString(),
* and GetLineView().
*
* Tests cover:
* - Edge cases: empty buffers, single lines, very long lines
* - Boundary conditions: first line, last line, out-of-bounds
* - Consistency: GetLineString vs GetLineView vs Rows()
* - Consistency: GetLineString vs GetLineView vs LinesForTests()
* - Performance: large files, many small operations
* - Correctness: special characters, newlines, unicode
*/
@@ -117,7 +117,7 @@ TEST (Migration_ManyEmptyLines)
// ============================================================================
// Consistency Tests: GetLineString vs GetLineView vs Rows()
// Consistency Tests: GetLineString vs GetLineView vs LinesForTests()
// ============================================================================
TEST (Migration_Consistency_AllMethods)
@@ -125,13 +125,13 @@ TEST (Migration_Consistency_AllMethods)
Buffer buf;
buf.insert_text(0, 0, std::string("abc\n123\nxyz"));
const auto &rows = buf.Rows();
const auto &rows = buf.LinesForTests();
ASSERT_EQ(buf.Nrows(), rows.size());
for (std::size_t i = 0; i < buf.Nrows(); ++i) {
std::string via_string = buf.GetLineString(i);
std::string via_rows = std::string(rows[i]);
// GetLineString and Rows() both strip newlines
// GetLineString and LinesForTests() both strip newlines
ASSERT_EQ(via_string, via_rows);
// GetLineView includes the raw range (with newlines if present)
// Just verify it's accessible
@@ -148,18 +148,18 @@ TEST (Migration_Consistency_AfterEdits)
// Edit: insert in middle
buf.insert_text(1, 2, std::string("XX"));
const auto &rows = buf.Rows();
const auto &rows = buf.LinesForTests();
ASSERT_EQ(buf.Nrows(), rows.size());
for (std::size_t i = 0; i < buf.Nrows(); ++i) {
// GetLineString and Rows() both strip newlines
// GetLineString and LinesForTests() both strip newlines
ASSERT_EQ(buf.GetLineString(i), std::string(rows[i]));
}
// Edit: delete line
buf.delete_row(1);
const auto &rows2 = buf.Rows();
const auto &rows2 = buf.LinesForTests();
ASSERT_EQ(buf.Nrows(), rows2.size());
for (std::size_t i = 0; i < buf.Nrows(); ++i) {
@@ -332,7 +332,7 @@ TEST (Migration_Stress_ManySmallInserts)
ASSERT_EQ(buf.GetLineString(1).size(), (std::size_t) 100);
// Verify consistency
const auto &rows = buf.Rows();
const auto &rows = buf.LinesForTests();
ASSERT_EQ(buf.GetLineString(1), std::string(rows[1]));
}
@@ -367,11 +367,11 @@ TEST (Migration_Stress_AlternatingInsertDelete)
}
// Verify consistency after many operations
const auto &rows = buf.Rows();
const auto &rows = buf.LinesForTests();
ASSERT_EQ(buf.Nrows(), rows.size());
for (std::size_t i = 0; i < buf.Nrows(); ++i) {
// GetLineString and Rows() both strip newlines
// GetLineString and LinesForTests() both strip newlines
ASSERT_EQ(buf.GetLineString(i), std::string(rows[i]));
}
}
+2 -2
View File
@@ -12,7 +12,7 @@ static std::string
to_string_rows(const Buffer &buf)
{
std::string out;
for (const auto &r: buf.Rows()) {
for (const auto &r: buf.LinesForTests()) {
out += static_cast<std::string>(r);
out.push_back('\n');
}
@@ -45,7 +45,7 @@ TEST(ReflowParagraph_IndentedBullets_PreserveStructure)
const int width = 80;
ASSERT_TRUE(Execute(ed, std::string("reflow-paragraph"), std::string(), width));
const auto &rows = buf->Rows();
const auto &rows = buf->LinesForTests();
const std::string result = to_string_rows(*buf);
// We should have 3 lines (plus possibly a trailing empty line)
+2 -2
View File
@@ -12,7 +12,7 @@ static std::string
to_string_rows(const Buffer &buf)
{
std::string out;
for (const auto &r: buf.Rows()) {
for (const auto &r: buf.LinesForTests()) {
out += static_cast<std::string>(r);
out.push_back('\n');
}
@@ -46,7 +46,7 @@ TEST(ReflowParagraph_NumberedList_HangingIndent)
const int width = 25;
ASSERT_TRUE(Execute(ed, std::string("reflow-paragraph"), std::string(), width));
const auto &rows = buf->Rows();
const auto &rows = buf->LinesForTests();
ASSERT_TRUE(!rows.empty());
const std::string dump = to_string_rows(*buf);
+2 -2
View File
@@ -12,7 +12,7 @@ static std::string
to_string_rows(const Buffer &buf)
{
std::string out;
for (const auto &r: buf.Rows()) {
for (const auto &r: buf.LinesForTests()) {
out += static_cast<std::string>(r);
out.push_back('\n');
}
@@ -50,7 +50,7 @@ TEST (ReflowUndo)
const std::string reflowed_dump = to_string_rows(*buf);
ASSERT_TRUE(reflowed_dump != original_dump);
ASSERT_TRUE(buf->Rows().size() > 1);
ASSERT_TRUE(buf->Nrows() > 1);
// Undo reflow
ASSERT_TRUE(Execute(ed, "undo", "", 1));
+1 -1
View File
@@ -127,7 +127,7 @@ TEST(SearchFlow_SearchReplace_EmptyWith_ReplacesAdjacentOverlappingMatches)
ASSERT_TRUE(h.Exec(CommandId::Newline));
ASSERT_TRUE(!ed.PromptActive());
ASSERT_EQ(std::string(b.Rows()[0]), std::string(""));
ASSERT_EQ(b.GetLineString(0), std::string(""));
}
+1 -1
View File
@@ -37,7 +37,7 @@ read_file_bytes(const std::string &path)
static std::string
buffer_bytes_via_views(const Buffer &b)
{
const auto &rows = b.Rows();
const auto &rows = b.LinesForTests();
std::string out;
for (std::size_t i = 0; i < rows.size(); i++) {
auto v = b.GetLineView(i);
+1 -1
View File
@@ -32,7 +32,7 @@ read_file_bytes(const std::string &path)
static std::string
buffer_bytes_via_views(const Buffer &b)
{
const auto &rows = b.Rows();
const auto &rows = b.LinesForTests();
std::string out;
for (std::size_t i = 0; i < rows.size(); i++) {
auto v = b.GetLineView(i);
+210 -210
View File
File diff suppressed because it is too large Load Diff
+1 -1
View File
@@ -11,7 +11,7 @@ static std::string
dump_buf(const Buffer &buf)
{
std::string out;
for (const auto &r: buf.Rows()) {
for (const auto &r: buf.LinesForTests()) {
out += static_cast<std::string>(r);
out.push_back('\n');
}