From fd58f88280e183716afda2e30c5542c2f5d1057c Mon Sep 17 00:00:00 2001 From: rfprod Date: Fri, 6 Nov 2026 21:20:13 +0400 Subject: [PATCH] perf(program): trim trailing spaces and honor overlap in `split_text` - [x] trim trailing spaces from the final line in `wrap_text`; - [x] keep the overlap start in `split_text` when no word boundary exists in range; - [x] expand `text_utils` tests to cover wrap and split edge cases and invalid arguments; - [x] replace manual `size_t` and int casts in tests with `static_cast` and typed assertions; - [x] specify `runs-on: ubuntu-24.04` in all action workflows; --- .github/workflows/trunk.yml | 2 +- .github/workflows/validate-pr.yml | 2 +- src/utils/text_utils.cpp | 14 +- tests/utils/text_utils_test.cpp | 382 ++++++++++++++++++++++++++---- tools/shell/format.sh | 83 ++++--- 5 files changed, 400 insertions(+), 83 deletions(-) diff --git a/.github/workflows/trunk.yml b/.github/workflows/trunk.yml index d5a1894..cbf5401 100644 --- a/.github/workflows/trunk.yml +++ b/.github/workflows/trunk.yml @@ -17,7 +17,7 @@ concurrency: jobs: test-build: - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 outputs: release: ${{ steps.src-changed.outputs.change == 'true' || steps.deps-changed.outputs.change == 'true' }} diff --git a/.github/workflows/validate-pr.yml b/.github/workflows/validate-pr.yml index 236539c..42e12b9 100644 --- a/.github/workflows/validate-pr.yml +++ b/.github/workflows/validate-pr.yml @@ -17,7 +17,7 @@ concurrency: jobs: premerge: - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 steps: - name: Checkout sources diff --git a/src/utils/text_utils.cpp b/src/utils/text_utils.cpp index a1667b4..583bdda 100644 --- a/src/utils/text_utils.cpp +++ b/src/utils/text_utils.cpp @@ -67,6 +67,9 @@ std::vector TextUtils::wrap_text(const std::string& text, const int } if (!remaining.empty()) { + // The first character is never a space here, so find_last_not_of always finds + // a non-space character and the final line is never empty. + remaining.erase(remaining.find_last_not_of(' ') + 1); result.push_back(remaining); } @@ -249,9 +252,14 @@ std::vector TextUtils::split_text(const std::string& text, const si if (sentence_start < end) { next = sentence_start; } else { - // No sentence start available: snap forward to the start of a word. - while (next < end && !is_space(text[next - 1])) { - next += 1; + // Snap forward to the first word start inside [next, end). + // If there is none, keep the overlap start so the overlap is honored. + size_t word_start = next; + while (word_start < end && !is_space(text[word_start - 1])) { + word_start += 1; + } + if (word_start < end) { + next = word_start; } } diff --git a/tests/utils/text_utils_test.cpp b/tests/utils/text_utils_test.cpp index e4a6eb6..005a74c 100644 --- a/tests/utils/text_utils_test.cpp +++ b/tests/utils/text_utils_test.cpp @@ -1,81 +1,371 @@ #include +#include +#include +#include +#include +#include #include -#include #include #include "utils/text_utils.hpp" TEST(TextUtils_wrap_text, no_wrap) { - std::string text = "This text will not be wrapped."; - int max_width = text.length() + 1; + const std::string text = "This text will not be wrapped."; + const int max_width = static_cast(text.length()) + 1; - std::vector result = TextUtils::wrap_text(text, max_width); + const std::vector result = TextUtils::wrap_text(text, max_width); - EXPECT_EQ(result.size(), 1); - EXPECT_EQ(result.at(0), text); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0], text); } TEST(TextUtils_wrap_text, wrap) { - std::string text = "This text value will be wrapped."; - int max_width = text.length() / 2; + const std::string text = "This text value will be wrapped."; + const int max_width = 16; - std::vector result = TextUtils::wrap_text(text, max_width); + const std::vector result = TextUtils::wrap_text(text, max_width); - size_t last_non_space{}; + ASSERT_EQ(result.size(), 2u); + EXPECT_EQ(result[0], "This text value"); + EXPECT_EQ(result[1], "will be wrapped."); +} - std::string first = text.substr(0, max_width); - last_non_space = first.find_last_not_of(' '); - first.resize(last_non_space + 1); +/** + * Argument validation. + */ - std::string second = text.substr(max_width); - last_non_space = second.find_last_not_of(' '); - second.resize(last_non_space + 1); +TEST(TextUtils_wrap_text, throws_on_zero_width) { + EXPECT_THROW(TextUtils::wrap_text("abc", 0), std::invalid_argument); +} - EXPECT_EQ(result.size(), 2); - EXPECT_EQ(result.at(0), first); - EXPECT_EQ(result.at(1), second); +TEST(TextUtils_wrap_text, throws_on_negative_width) { + EXPECT_THROW(TextUtils::wrap_text("abc", -1), std::invalid_argument); + EXPECT_THROW(TextUtils::wrap_text("abc", INT_MIN), std::invalid_argument); } -TEST(TextUtils_split_text, no_split) { - std::string text = "This text will not be chunked."; - size_t chunk_size = text.length() + 1; - size_t overlap = 0; +/** + * Empty and whitespace-only input (covers the first == npos branch). + */ - std::vector chunks = TextUtils::split_text(text, chunk_size, overlap); +TEST(TextUtils_wrap_text, empty_input_returns_no_lines) { + EXPECT_TRUE(TextUtils::wrap_text("", 10).empty()); +} - EXPECT_EQ(chunks.size(), 1); - EXPECT_EQ(chunks.at(0), text); +TEST(TextUtils_wrap_text, only_spaces_returns_no_lines) { + EXPECT_TRUE(TextUtils::wrap_text(" ", 3).empty()); } -TEST(TextUtils_split_text, split_no_overlap) { - std::string text = "This text value will be chunked."; - size_t chunk_size = text.length() / 2; - size_t overlap = 0; +TEST(TextUtils_wrap_text, leading_spaces_are_removed) { + const std::vector result = TextUtils::wrap_text(" hello world", 20); + + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0], "hello world"); +} + +/** + * Width boundaries. + */ + +TEST(TextUtils_wrap_text, text_exactly_max_width_is_not_wrapped) { + const std::vector result = TextUtils::wrap_text("abcdefghij", 10); + + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0], "abcdefghij"); +} + +TEST(TextUtils_wrap_text, width_one_splits_every_character) { + const std::vector result = TextUtils::wrap_text("abc", 1); + + ASSERT_EQ(result.size(), 3u); + EXPECT_EQ(result[0], "a"); + EXPECT_EQ(result[1], "b"); + EXPECT_EQ(result[2], "c"); +} + +TEST(TextUtils_wrap_text, space_exactly_at_width_is_the_break_point) { + const std::vector result = TextUtils::wrap_text("abcd efgh", 4); + + ASSERT_EQ(result.size(), 2u); + EXPECT_EQ(result[0], "abcd"); + EXPECT_EQ(result[1], "efgh"); +} + +/** + * Hard breaks (covers the wrap_position == npos branch). + */ + +TEST(TextUtils_wrap_text, word_longer_than_width_is_split) { + const std::vector result = TextUtils::wrap_text("abcdefghij", 4); + + ASSERT_EQ(result.size(), 3u); + EXPECT_EQ(result[0], "abcd"); + EXPECT_EQ(result[1], "efgh"); + EXPECT_EQ(result[2], "ij"); +} + +TEST(TextUtils_wrap_text, long_word_followed_by_space_is_hard_broken) { + const std::vector result = TextUtils::wrap_text("abcde fgh", 4); + + ASSERT_EQ(result.size(), 3u); + EXPECT_EQ(result[0], "abcd"); + EXPECT_EQ(result[1], "e"); + EXPECT_EQ(result[2], "fgh"); +} + +/** + * Whitespace handling around breaks (covers the next_start == npos branch). + */ + +TEST(TextUtils_wrap_text, multiple_spaces_at_break_are_collapsed) { + const std::vector result = TextUtils::wrap_text("aaa bbb", 5); - std::vector chunks = TextUtils::split_text(text, chunk_size, overlap); + ASSERT_EQ(result.size(), 2u); + EXPECT_EQ(result[0], "aaa"); + EXPECT_EQ(result[1], "bbb"); +} + +TEST(TextUtils_wrap_text, trailing_spaces_after_last_break_produce_no_extra_line) { + const std::vector result = TextUtils::wrap_text("abcde ", 5); + + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0], "abcde"); +} + +TEST(TextUtils_wrap_text, trailing_spaces_on_unwrapped_text_are_removed) { + const std::vector result = TextUtils::wrap_text("hello ", 20); + + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0], "hello"); +} + +/** + * Multi-line behaviour. + */ + +TEST(TextUtils_wrap_text, multiple_lines_with_hard_coded_expectations) { + const std::vector result = TextUtils::wrap_text("The quick brown fox jumps over the lazy dog", 10); + + const std::vector expected = {"The quick", "brown fox", "jumps over", "the lazy", "dog"}; + EXPECT_EQ(result, expected); +} + +TEST(TextUtils_wrap_text, every_line_respects_width_and_has_no_edge_spaces) { + const std::string text = "The quick brown fox jumps over the lazy dog"; + + for (int width = 1; width <= static_cast(text.length()) + 2; width += 1) { + const std::vector result = TextUtils::wrap_text(text, width); - size_t last_non_space{}; + for (const std::string& line : result) { + SCOPED_TRACE("width=" + std::to_string(width) + " line=\"" + line + "\""); + EXPECT_FALSE(line.empty()); + EXPECT_LE(line.length(), static_cast(width)); + EXPECT_NE(line.front(), ' '); + EXPECT_NE(line.back(), ' '); + } + } +} + +TEST(TextUtils_wrap_text, rejoined_lines_reproduce_original_words) { + const std::string text = "The quick brown fox jumps over the lazy dog"; + const int longest_word = 5; - std::string first = text.substr(0, chunk_size); - last_non_space = first.find_last_not_of(' '); - first.resize(last_non_space + 1); + for (int width = longest_word; width <= static_cast(text.length()); width += 1) { + const std::vector result = TextUtils::wrap_text(text, width); - std::string second = text.substr(chunk_size - overlap); - last_non_space = second.find_last_not_of(' '); - second.resize(last_non_space + 1); + std::string rejoined; + for (size_t i = 0; i < result.size(); ++i) { + if (i > 0) { + rejoined += ' '; + } + rejoined += result[i]; + } - EXPECT_EQ(chunks.size(), 2); - EXPECT_EQ(chunks.at(0), first); - EXPECT_EQ(chunks.at(1), second); + SCOPED_TRACE("width=" + std::to_string(width)); + EXPECT_EQ(rejoined, text); + } +} + +/** + * Characters that are not spaces. + */ + +TEST(TextUtils_wrap_text, tabs_are_not_break_points) { + const std::vector result = TextUtils::wrap_text("abc\tdef", 4); + + ASSERT_EQ(result.size(), 2u); + EXPECT_EQ(result[0], "abc\t"); + EXPECT_EQ(result[1], "def"); +} + +/** + * Large input (smoke test for correctness and for the O(n^2 / width) copying). + */ + +TEST(TextUtils_wrap_text, large_input_wraps_into_expected_number_of_lines) { + constexpr size_t word_count = 10000; + + std::ostringstream builder; + for (size_t i = 0; i < word_count; i += 1) { + if (i > 0) { + builder << ' '; + } + builder << "ab"; + } + + // Each line holds three "ab" words (8 characters); the width of 10 cannot fit a fourth. + // 10000 words yield 3333 full lines plus one final line containing a single word. + const std::vector result = TextUtils::wrap_text(builder.str(), 10); + + ASSERT_EQ(result.size(), 3334u); + EXPECT_EQ(result.front(), "ab ab ab"); + EXPECT_EQ(result.back(), "ab"); +} + +// split_text + +TEST(TextUtils_split_text, no_split) { + const std::string text = "This text will not be chunked."; + const auto chunks = TextUtils::split_text(text, text.length() + 1, 0); + + ASSERT_EQ(chunks.size(), 1u); + EXPECT_EQ(chunks[0], text); +} + +TEST(TextUtils_split_text, split_no_overlap) { + const std::string text = "This text value will be chunked."; + const auto chunks = TextUtils::split_text(text, 16, 0); + + ASSERT_EQ(chunks.size(), 2u); + EXPECT_EQ(chunks[0], "This text value"); + EXPECT_EQ(chunks[1], "will be chunked."); } TEST(TextUtils_split_text, split_with_overlap) { - std::string text = "This text value is chunked with overlap."; - size_t chunk_size = text.length() / 2; - size_t overlap = 4; + const std::string text = "This text value is chunked with overlap."; + const auto chunks = TextUtils::split_text(text, 20, 4); + + ASSERT_EQ(chunks.size(), 3u); + EXPECT_EQ(chunks[0], "This text value is"); + EXPECT_EQ(chunks[1], "is chunked with"); + EXPECT_EQ(chunks[2], "with overlap."); +} + +TEST(TextUtils_split_text, empty_input_returns_no_chunks) { + EXPECT_TRUE(TextUtils::split_text("", 10, 0).empty()); +} + +TEST(TextUtils_split_text, whitespace_only_input_returns_no_chunks) { + EXPECT_TRUE(TextUtils::split_text(" \t\n ", 10, 0).empty()); +} + +TEST(TextUtils_split_text, leading_whitespace_is_trimmed) { + const auto chunks = TextUtils::split_text(" Hello world", 100, 0); + + ASSERT_EQ(chunks.size(), 1u); + EXPECT_EQ(chunks[0], "Hello world"); +} + +TEST(TextUtils_split_text, trailing_whitespace_is_trimmed) { + const auto chunks = TextUtils::split_text("Hello world ", 100, 0); + + ASSERT_EQ(chunks.size(), 1u); + EXPECT_EQ(chunks[0], "Hello world"); +} + +TEST(TextUtils_split_text, text_exactly_chunk_size_is_one_chunk) { + const auto chunks = TextUtils::split_text("abcdefghij", 10, 0); + + ASSERT_EQ(chunks.size(), 1u); + EXPECT_EQ(chunks[0], "abcdefghij"); +} + +TEST(TextUtils_split_text, zero_chunk_size_throws) { + EXPECT_THROW(TextUtils::split_text("abc", 0, 0), std::invalid_argument); +} + +TEST(TextUtils_split_text, overlap_equal_to_chunk_size_throws) { + EXPECT_THROW(TextUtils::split_text("abc", 5, 5), std::invalid_argument); +} + +TEST(TextUtils_split_text, overlap_greater_than_chunk_size_throws) { + EXPECT_THROW(TextUtils::split_text("abc", 5, 6), std::invalid_argument); +} + +TEST(TextUtils_split_text, hard_split_when_no_whitespace) { + const auto chunks = TextUtils::split_text("abcdefghijklmnopqrstuvwxyz", 10, 0); + + ASSERT_EQ(chunks.size(), 3u); + EXPECT_EQ(chunks[0], "abcdefghij"); + EXPECT_EQ(chunks[1], "klmnopqrst"); + EXPECT_EQ(chunks[2], "uvwxyz"); +} + +TEST(TextUtils_split_text, overlap_honored_when_no_whitespace) { + const auto chunks = TextUtils::split_text("abcdefghijklmnopqrstuvwxyz", 10, 3); + + ASSERT_EQ(chunks.size(), 4u); + EXPECT_EQ(chunks[0], "abcdefghij"); + EXPECT_EQ(chunks[1], "hijklmnopq"); + EXPECT_EQ(chunks[2], "opqrstuvwx"); + EXPECT_EQ(chunks[3], "vwxyz"); +} + +TEST(TextUtils_split_text, prefers_sentence_boundary) { + const auto chunks = TextUtils::split_text("First sentence is here. Second one follows now.", 30, 0); + + ASSERT_EQ(chunks.size(), 2u); + EXPECT_EQ(chunks[0], "First sentence is here."); + EXPECT_EQ(chunks[1], "Second one follows now."); +} + +TEST(TextUtils_split_text, abbreviation_is_not_a_sentence_boundary) { + const auto chunks = TextUtils::split_text("Call Dr. Lee now please.", 12, 0); + + ASSERT_EQ(chunks.size(), 2u); + EXPECT_EQ(chunks[0], "Call Dr. Lee"); + EXPECT_EQ(chunks[1], "now please."); +} + +TEST(TextUtils_split_text, no_chunk_exceeds_chunk_size) { + const std::string text = "The quick brown fox. Jumps over the lazy dog! Does it work? Yes, it does."; + + for (size_t chunk_size = 1; chunk_size <= 20; ++chunk_size) { + for (size_t overlap = 0; overlap < chunk_size; ++overlap) { + const auto chunks = TextUtils::split_text(text, chunk_size, overlap); + ASSERT_FALSE(chunks.empty()); + + for (const auto& chunk : chunks) { + EXPECT_FALSE(chunk.empty()); + EXPECT_LE(chunk.size(), chunk_size) << "chunk_size=" << chunk_size << " overlap=" << overlap; + EXPECT_NE(chunk.front(), ' '); + EXPECT_NE(chunk.back(), ' '); + } + } + } +} + +namespace { + +std::string remove_spaces(std::string s) { + s.erase(std::remove(s.begin(), s.end(), ' '), s.end()); + return s; +} + +} // namespace + +TEST(TextUtils_split_text, no_overlap_keeps_every_non_space_character_once) { + const std::string text = "The quick brown fox jumps over the lazy dog and keeps running far away."; + const std::string expected = remove_spaces(text); + + for (size_t chunk_size = 1; chunk_size <= 25; ++chunk_size) { + const auto chunks = TextUtils::split_text(text, chunk_size, 0); - std::vector chunks = TextUtils::split_text(text, chunk_size, overlap); + std::string joined; + for (const auto& chunk : chunks) { + joined += chunk; + } - EXPECT_EQ(chunks.size(), 3); + EXPECT_EQ(remove_spaces(joined), expected) << "chunk_size=" << chunk_size; + } } diff --git a/tools/shell/format.sh b/tools/shell/format.sh index 979e58e..b6fedf5 100644 --- a/tools/shell/format.sh +++ b/tools/shell/format.sh @@ -3,38 +3,57 @@ set -euo pipefail # Change to repo root (handles running from any directory) -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" -cd "$REPO_ROOT" +ensure_repo_root() { + local SCRIPT_DIR + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -echo "Running from: $(pwd)" -echo "---" -echo "Directory structure [src]:" -ls -la src/ -echo "---" -echo "Directory structure [tests]:" -ls -la tests/ -echo "---" -echo "All .cpp and .hpp files [src]:" -find src tests -type f \( -name "*.cpp" -o -name "*.hpp" \) -echo "---" -echo "All .cpp and .hpp files [tests]:" -find tests -type f \( -name "*.cpp" -o -name "*.hpp" \) -echo "---" + local REPO_ROOT + REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" + cd "$REPO_ROOT" +} +ensure_repo_root -echo "Checking with clang-format..." -find src tests -type f \( -name "*.cpp" -o -name "*.hpp" \) -exec clang-format -i -Werror {} + +print_diag() { + echo "Running from: $(pwd)" + echo "---" + echo "Directory structure [src]:" + ls -la src/ + echo "---" + echo "Directory structure [tests]:" + ls -la tests/ + echo "---" + echo "All .cpp and .hpp files [src]:" + find src tests -type f \( -name "*.cpp" -o -name "*.hpp" \) + echo "---" + echo "All .cpp and .hpp files [tests]:" + find tests -type f \( -name "*.cpp" -o -name "*.hpp" \) + echo "---" +} +print_diag -echo "Checking with cppcheck..." -cppcheck --std=c++20 \ - --enable=all \ - --error-exitcode=1 \ - --suppress=missingIncludeSystem \ - --suppress=unusedLabel \ - -I src/ \ - -I src/interface \ - -I src/ollama \ - -I src/storage \ - -I src/utils \ - -I tests/ \ - src/ tests/ +clang_format_check() { + local VERSION + VERSION=$(clang-format --version) + echo "Checking with clang-format [${VERSION}]..." + find src tests -type f \( -name "*.cpp" -o -name "*.hpp" \) -exec clang-format -i -Werror {} + +} +clang_format_check + +cppcheck_check() { + local VERSION + VERSION=$(cppcheck --version) + echo "Checking with cppcheck [${VERSION}]..." + cppcheck --std=c++20 \ + --enable=all \ + --error-exitcode=1 \ + --suppress=missingIncludeSystem \ + --suppress=unusedLabel \ + -I src/ \ + -I src/interface \ + -I src/ollama \ + -I src/storage \ + -I src/utils \ + -I tests/ \ + src/ tests/ +} +cppcheck_check