From 582c3c56b218b9be9745efc69c2eb282bbe99b67 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 28 Jan 2024 18:23:08 +0100 Subject: Do not add trailing newline to non-scrolling fifo buffers Internally, all lines have a trailing "\n". Buffers created empty (like fifo buffers) start with a single line. When reading data into fifo buffers, we insert *before* the last line's trailing newline ("last newline"). This enables autoscrolling (enabled with "edit -scroll") as long as the cursor is on the last newline. When autoscrolling is disabled, we have a special case to insert *after* the last newline. This means that a cursor on that newline won't be moved. Then we transplant the newline character from the beginning to the end of the buffer. This special case happens only on the very first fifo read; on subsequent reads, the cursor at position 1.1 will not be moved anway because insertions happen below 1.1. Since we always insert (effectively) before the last newline, fifo buffers have a trailing empty line. For autoscrolling buffers this seems correct; it gives users an obvious way to toggle autoscrolling. For non-scrolling buffers the newline is redundant. Remove it. This requires keeping track of whether the last newline comes from the fifo, or was added by us. The shortest fix I could find is to always append to the buffer if not scrolling, and then delete the added newline character if applicable. m_buffer.insert(m_scroll ? pos : m_buffer.next(pos), StringView(data, data+count)); if (not m_scroll and not m_had_trailing_newline) m_buffer.erase(pos, m_buffer.next(pos)); maybe that's the best fix overall; but erasing at the end seems better than erasing in the middle, so do that whenever possible. Reported in https://lists.sr.ht/~mawww/kakoune/%3CZbTK7qit9nzvrMkx@gmail.com%3E --- src/buffer_utils.cc | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) (limited to 'src/buffer_utils.cc') diff --git a/src/buffer_utils.cc b/src/buffer_utils.cc index 6de07c71..21ec7092 100644 --- a/src/buffer_utils.cc +++ b/src/buffer_utils.cc @@ -205,7 +205,7 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll m_buffer.flags() &= ~(Buffer::Flags::Fifo | Buffer::Flags::NoUndo); } - void read_fifo() const + void read_fifo() { kak_assert(m_buffer.flags() & Buffer::Flags::Fifo); @@ -234,20 +234,21 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll } auto pos = m_buffer.back_coord(); - const bool prevent_scrolling = pos == BufferCoord{0,0} and not m_scroll; - if (prevent_scrolling) + const bool is_first = pos == BufferCoord{0,0}; + if (not m_scroll and (is_first or m_had_trailing_newline)) pos = m_buffer.next(pos); - m_buffer.insert(pos, StringView(data, data+count)); + pos = m_buffer.insert(pos, StringView(data, data+count)).end; - if (prevent_scrolling) + bool have_trailing_newline = (data[count-1] == '\n'); + if (not m_scroll) { - m_buffer.erase({0,0}, m_buffer.next({0,0})); - // in the other case, the buffer will have automatically - // inserted a \n to guarantee its invariant. - if (data[count-1] == '\n') - m_buffer.insert(m_buffer.end_coord(), "\n"); + if (is_first) + m_buffer.erase({0,0}, m_buffer.next({0,0})); + else if (not m_had_trailing_newline and have_trailing_newline) + m_buffer.erase(pos, m_buffer.next(pos)); } + m_had_trailing_newline = have_trailing_newline; } while (++loop < max_loop and fd_readable(fifo)); } @@ -263,6 +264,7 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll Buffer& m_buffer; bool m_scroll; + bool m_had_trailing_newline; }; buffer->values()[fifo_watcher_id] = Value(Meta::Type{}, fd, *buffer, scroll); -- cgit v1.2.3 From 04a96b059faac8100a291e56bfbdb1962d53d4e1 Mon Sep 17 00:00:00 2001 From: Maxime Coste Date: Tue, 6 Feb 2024 21:57:17 +1100 Subject: Use different hash algorithms for strings and file hashing For hash map, using fnv1a is faster as it is a much simpler algorithm we can afford to inline. For files murmur3 should win as it processes bytes 4 by 4. --- src/buffer_utils.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/buffer_utils.cc') diff --git a/src/buffer_utils.cc b/src/buffer_utils.cc index 21ec7092..be255e4a 100644 --- a/src/buffer_utils.cc +++ b/src/buffer_utils.cc @@ -140,7 +140,7 @@ decltype(auto) parse_file(StringView filename, Func&& func) const bool crlf = has_crlf and not has_lf; auto eolformat = crlf ? EolFormat::Crlf : EolFormat::Lf; - FsStatus fs_status{file.st.st_mtim, file.st.st_size, hash_data(file.data, file.st.st_size)}; + FsStatus fs_status{file.st.st_mtim, file.st.st_size, murmur3(file.data, file.st.st_size)}; return func(parse_lines(pos, end, eolformat), bom, eolformat, fs_status); } -- cgit v1.2.3 From 22b1d4ec4a3d2ae6a31f2395b43a209fc4d54653 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 17 Feb 2024 22:32:05 +0100 Subject: Try to fix crash due to fifo read regression I saw a crash when running git log --oneline %arg{@} hook -once buffer NormalIdle .* %{ execute-keys -draft \ %{gk!} \ %{git diff --quiet || echo "Unstaged changes";} \ %{git diff --quiet --cached || echo "Staged changes";} \ } Backtrace (I still have GDB attached): #4 0x00006502c740b13f in Kakoune::operator- (rhs=..., lhs=...) at /home/johannes/git/kakoune/src/units.hh:33 33 { return RealType(lhs.m_value - rhs.m_value); } (gdb) up #5 Kakoune::Buffer::next (coord=..., this=0x6502c90d7ff0) at /home/johannes/git/kakoune/src/buffer.inl.hh:18 18 if (coord.column < m_lines[coord.line].length() - 1) (gdb) up #6 FifoWatcher::read_fifo (this=0x6502c90d9e48) at buffer_utils.cc:252 252 m_buffer.erase(pos, m_buffer.next(pos)); This was introduced in 582c3c56b (Do not add trailing newline to non-scrolling fifo buffers, 2024-01-28). The problem seems to be that we call "m_buffer.next()" on a position that is past-end the buffer, so m_lines[coord.line] is out-of-bounds. Fix it. For some reason I have not managed to reproduce the crash, not even with sanitize=address. There might be another problem: m_had_trailing_newline is intentionally uninitialized because it is supposed to be read only on the second read() with a positive return value. Unfortunately I think it's possible that e.g. a NormalIdle hook inserts some text before the first positive read(). Then, this line const bool is_first = pos == BufferCoord{0,0}; if (not m_scroll and (is_first or m_had_trailing_newline)) pos = m_buffer.next(pos); will read uninitialized "m_had_trailing_newline". Fix that too, to be on the safe side. Sadly I don't have a test for this one either so I'm not sure. --- src/buffer_utils.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/buffer_utils.cc') diff --git a/src/buffer_utils.cc b/src/buffer_utils.cc index be255e4a..996f6be5 100644 --- a/src/buffer_utils.cc +++ b/src/buffer_utils.cc @@ -246,7 +246,7 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll if (is_first) m_buffer.erase({0,0}, m_buffer.next({0,0})); else if (not m_had_trailing_newline and have_trailing_newline) - m_buffer.erase(pos, m_buffer.next(pos)); + m_buffer.erase(m_buffer.prev(pos), pos); } m_had_trailing_newline = have_trailing_newline; } @@ -264,7 +264,7 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll Buffer& m_buffer; bool m_scroll; - bool m_had_trailing_newline; + bool m_had_trailing_newline = false; }; buffer->values()[fifo_watcher_id] = Value(Meta::Type{}, fd, *buffer, scroll); -- cgit v1.2.3 From 3d5a0c672e6f3cf87944b33712e17531aa42c607 Mon Sep 17 00:00:00 2001 From: Maxime Coste Date: Wed, 28 Feb 2024 19:00:51 +1100 Subject: Templatize StringData::create This improves performance by letting the compiler optimize most use cases where string count and length are known are compile time. --- src/buffer_utils.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/buffer_utils.cc') diff --git a/src/buffer_utils.cc b/src/buffer_utils.cc index 996f6be5..effb7cc7 100644 --- a/src/buffer_utils.cc +++ b/src/buffer_utils.cc @@ -97,12 +97,12 @@ static BufferLines parse_lines(const char* pos, const char* end, EolFormat eolfo if ((eol - pos) >= std::numeric_limits::max()) throw runtime_error("line is too long"); - lines.emplace_back(StringData::create({{pos, eol - (eolformat == EolFormat::Crlf and eol != end ? 1 : 0)}, "\n"})); + lines.emplace_back(StringData::create(StringView{pos, eol - (eolformat == EolFormat::Crlf and eol != end ? 1 : 0)}, "\n")); pos = eol + 1; } if (lines.empty()) - lines.emplace_back(StringData::create({"\n"})); + lines.emplace_back(StringData::create("\n")); return lines; } @@ -179,12 +179,12 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll { buffer->flags() |= Buffer::Flags::NoUndo | flags; buffer->values().clear(); - buffer->reload({StringData::create({"\n"})}, ByteOrderMark::None, EolFormat::Lf, {InvalidTime, {}, {}}); + buffer->reload({StringData::create("\n")}, ByteOrderMark::None, EolFormat::Lf, {InvalidTime, {}, {}}); } else buffer = buffer_manager.create_buffer( std::move(name), flags | Buffer::Flags::Fifo | Buffer::Flags::NoUndo, - {StringData::create({"\n"})}, ByteOrderMark::None, EolFormat::Lf, {InvalidTime, {}, {}}); + {StringData::create("\n")}, ByteOrderMark::None, EolFormat::Lf, {InvalidTime, {}, {}}); struct FifoWatcher : FDWatcher { -- cgit v1.2.3 From 068af3d9d4d7fd8f442ec365d3c9f6a3c02c64a7 Mon Sep 17 00:00:00 2001 From: Maxime Coste Date: Wed, 28 Feb 2024 19:05:45 +1100 Subject: Use std::find when detecting end of line format --- src/buffer_utils.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'src/buffer_utils.cc') diff --git a/src/buffer_utils.cc b/src/buffer_utils.cc index effb7cc7..76910483 100644 --- a/src/buffer_utils.cc +++ b/src/buffer_utils.cc @@ -132,13 +132,9 @@ decltype(auto) parse_file(StringView filename, Func&& func) } bool has_crlf = false, has_lf = false; - for (auto it = pos; it != end; ++it) - { - if (*it == '\n') - ((it != pos and *(it-1) == '\r') ? has_crlf : has_lf) = true; - } - const bool crlf = has_crlf and not has_lf; - auto eolformat = crlf ? EolFormat::Crlf : EolFormat::Lf; + for (auto it = std::find(pos, end, '\n'); it != end; it = std::find(it+1, end, '\n')) + ((it != pos and *(it-1) == '\r') ? has_crlf : has_lf) = true; + auto eolformat = (has_crlf and not has_lf) ? EolFormat::Crlf : EolFormat::Lf; FsStatus fs_status{file.st.st_mtim, file.st.st_size, murmur3(file.data, file.st.st_size)}; return func(parse_lines(pos, end, eolformat), bom, eolformat, fs_status); -- cgit v1.2.3 From e8ad943532f2211dcbaeb2e75f71ef2f6516dd7f Mon Sep 17 00:00:00 2001 From: Maxime Coste Date: Fri, 19 Apr 2024 17:04:41 +1000 Subject: Ensure re-used fifo buffers makes that buffer the latest opened --- src/buffer_utils.cc | 1 + 1 file changed, 1 insertion(+) (limited to 'src/buffer_utils.cc') diff --git a/src/buffer_utils.cc b/src/buffer_utils.cc index 76910483..6a66b000 100644 --- a/src/buffer_utils.cc +++ b/src/buffer_utils.cc @@ -176,6 +176,7 @@ Buffer* create_fifo_buffer(String name, int fd, Buffer::Flags flags, bool scroll buffer->flags() |= Buffer::Flags::NoUndo | flags; buffer->values().clear(); buffer->reload({StringData::create("\n")}, ByteOrderMark::None, EolFormat::Lf, {InvalidTime, {}, {}}); + buffer_manager.make_latest(*buffer); } else buffer = buffer_manager.create_buffer( -- cgit v1.2.3