refactor(clp-s): Delete unused `clp_s::StringUtils` functions; Remove unused `Utils.hpp` includes. #1269

gibber9809 · 2025-08-27T18:51:06Z

Description

This PR follows up on #1163 and #1103 by deleting the remainder of the StringUtils methods that have equivalent implementations in clp; it is now safe to remove the rest of these copied methods since we now use the core clp parsing and search code directly in clp-s. We also clean up unused references to clp-s' Utils.hpp header (most of these remaining references were previously used in order to include the now-deleted methods).

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Validated that unit tests still pass.

Summary by CodeRabbit

New Features
- No user-facing features added in this release.
Refactor
- Reduced internal string-helper surface and trimmed unnecessary internal dependencies to simplify the codebase.
Chores
- Cleaned up includes and removed unused helpers to improve maintainability and potentially speed builds.
Notes
- No changes to behaviour, public APIs, or user workflows; integrations remain unaffected.

coderabbitai · 2025-08-27T18:51:13Z

Walkthrough

This change removes three StringUtils helpers from Utils.hpp/Utils.cpp, deletes their implementation, removes redundant #include "Utils.hpp" and one using in several clp_s files, and adds a targeted #include "Utils.hpp" in search/QueryRunner.cpp. Public signatures outside StringUtils are unchanged.

Changes

Cohort / File(s)	Change summary
StringUtils API removal `components/core/src/clp_s/Utils.hpp`, `components/core/src/clp_s/Utils.cpp`	Removed declarations: `is_delim(char)`, `could_be_multi_digit_hex_value(std::string const&, size_t, size_t)`, and `get_bounds_of_next_var(std::string const&, size_t&, size_t&)`; removed `get_bounds_of_next_var` implementation. `escape_json_string` retained.
Utils.hpp include removals in clp_s `components/core/src/clp_s/ArchiveReader.hpp`, `components/core/src/clp_s/DictionaryEntry.cpp`, `components/core/src/clp_s/DictionaryReader.hpp`, `components/core/src/clp_s/JsonParser.hpp`, `components/core/src/clp_s/Schema.hpp`, `components/core/src/clp_s/TimestampDictionaryWriter.cpp`, `components/core/src/clp_s/clp-s.cpp`	Dropped `#include "Utils.hpp"` from listed files; in `clp-s.cpp` also removed `using clp_s::StringUtils;`. No other functional changes.
Search query include addition `components/core/src/clp_s/search/QueryRunner.cpp`	Added `#include "Utils.hpp"`; no logic or API changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

refactor(clp-s): Replace clp_s::StringUtils function calls with clp::string_utils equivalents and remove redundant implementations. #1103 — Refactors/removes duplicate StringUtils and replaces usages with clp::string_utils; overlaps with removal of StringUtils helpers and Utils.hpp includes.
refactor(clp-s): Divide clp-s search AST code so that it can easily be turned into a library. #754 — Moves StringUtils functionality into search utilities and trims Utils.hpp; closely aligned with include removals and API reductions here.
feat(clp-s)!: Use core clp parsing and search code in clp-s; Bump archive version to 0.4.0. #1163 — Migrates clp-s to shared parsing/search code, altering/removing the same StringUtils functions targeted in this PR.

Suggested reviewers

wraymo
kirkrodrigues
haiqi96

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 799688d and 4c868ea.

📒 Files selected for processing (2)

components/core/src/clp_s/JsonParser.hpp (0 hunks)
components/core/src/clp_s/search/QueryRunner.cpp (1 hunks)

💤 Files with no reviewable changes (1)

components/core/src/clp_s/JsonParser.hpp

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

Prefer false == <expression> rather than !<expression>.

Files:

components/core/src/clp_s/search/QueryRunner.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
GitHub Check: ubuntu-jammy-static-linked-bins
GitHub Check: package-image
GitHub Check: manylinux_2_28-x86_64-static-linked-bins
GitHub Check: musllinux_1_2-x86_64-static-linked-bins
GitHub Check: ubuntu-jammy-lint
GitHub Check: ubuntu-jammy-dynamic-linked-bins
GitHub Check: centos-stream-9-dynamic-linked-bins
GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
GitHub Check: centos-stream-9-static-linked-bins
GitHub Check: build-macos (macos-15, false)
GitHub Check: build-macos (macos-15, true)

🔇 Additional comments (1)

components/core/src/clp_s/search/QueryRunner.cpp (1)
14-14: Unable to locate any usage of EvaluatedValue in QueryRunner.cpp; the include is only needed for the custom bit_cast. Replace with std::bit_cast and switch the include accordingly.
-#include "../Utils.hpp"
+#include <bit>

… 
uint64_t tmp_uint = std::bit_cast<uint64_t>(tmp_int);

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled by default for public repositories
Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c7804f2 and 799688d.

📒 Files selected for processing (10)

components/core/src/clp_s/ArchiveReader.hpp (0 hunks)
components/core/src/clp_s/DictionaryEntry.cpp (0 hunks)
components/core/src/clp_s/DictionaryReader.hpp (0 hunks)
components/core/src/clp_s/JsonParser.hpp (0 hunks)
components/core/src/clp_s/Schema.hpp (0 hunks)
components/core/src/clp_s/TimestampDictionaryWriter.cpp (0 hunks)
components/core/src/clp_s/Utils.cpp (0 hunks)
components/core/src/clp_s/Utils.hpp (0 hunks)
components/core/src/clp_s/clp-s.cpp (0 hunks)
components/core/src/clp_s/search/QueryRunner.cpp (1 hunks)

💤 Files with no reviewable changes (9)

components/core/src/clp_s/DictionaryEntry.cpp
components/core/src/clp_s/ArchiveReader.hpp
components/core/src/clp_s/JsonParser.hpp
components/core/src/clp_s/TimestampDictionaryWriter.cpp
components/core/src/clp_s/clp-s.cpp
components/core/src/clp_s/Schema.hpp
components/core/src/clp_s/DictionaryReader.hpp
components/core/src/clp_s/Utils.cpp
components/core/src/clp_s/Utils.hpp

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

Prefer false == <expression> rather than !<expression>.

Files:

components/core/src/clp_s/search/QueryRunner.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

GitHub Check: centos-stream-9-dynamic-linked-bins
GitHub Check: centos-stream-9-static-linked-bins
GitHub Check: ubuntu-jammy-dynamic-linked-bins
GitHub Check: ubuntu-jammy-static-linked-bins
GitHub Check: ubuntu-jammy-lint
GitHub Check: build-macos (macos-15, false)
GitHub Check: build-macos (macos-15, true)
GitHub Check: lint-check (macos-15)

components/core/src/clp_s/search/QueryRunner.cpp

Bill-hbrhbr

I'm thinking maybe you can move the leftover clp-s specific string utils to the string_utils module, so that I can move it to ystdlib in the near future.

gibber9809 · 2025-09-03T14:50:09Z

I'm thinking maybe you can move the leftover clp-s specific string utils to the string_utils module, so that I can move it to ystdlib in the near future.

Sure, that's a good idea for a follow-up PR after this one.

Bill-hbrhbr

lgtm

kirkrodrigues

Deferring to @Bill-hbrhbr's review.

For the PR title, how about:

refactor(clp-s): Delete unused `clp_s::StringUtils` functions; Remove unused `Utils.hpp` includes.

gibber9809 added 2 commits August 27, 2025 18:22

Delete unused StringUtils methods; Remove unused includes of Utils.hpp

71b491f

Remove more unused includes of Utils.hpp

799688d

gibber9809 requested review from a team and wraymo as code owners August 27, 2025 18:51

coderabbitai bot reviewed Aug 27, 2025

View reviewed changes

components/core/src/clp_s/search/QueryRunner.cpp Show resolved Hide resolved

gibber9809 added 2 commits August 28, 2025 11:26

Merge branch 'main' into delete-unused-string-utils

205eaa7

Merge branch 'main' into delete-unused-string-utils

b1a23c7

Bill-hbrhbr reviewed Sep 3, 2025

View reviewed changes

Bill-hbrhbr approved these changes Sep 4, 2025

View reviewed changes

kirkrodrigues approved these changes Sep 8, 2025

View reviewed changes

gibber9809 changed the title ~~refactor(clp-s): Delete unused clp_s::StringUtils methods; Remove unused includes of Utils.hpp.~~ refactor(clp-s): Delete unused clp_s::StringUtils functions; Remove unused Utils.hpp includes. Sep 8, 2025

Merge branch 'main' into delete-unused-string-utils

4c868ea

gibber9809 merged commit a1382e3 into y-scope:main Sep 8, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(clp-s): Delete unused `clp_s::StringUtils` functions; Remove unused `Utils.hpp` includes. #1269

refactor(clp-s): Delete unused `clp_s::StringUtils` functions; Remove unused `Utils.hpp` includes. #1269

Uh oh!

gibber9809 commented Aug 27, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Aug 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Bill-hbrhbr left a comment

Uh oh!

gibber9809 commented Sep 3, 2025

Uh oh!

Bill-hbrhbr left a comment

Uh oh!

kirkrodrigues left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(clp-s): Delete unused clp_s::StringUtils functions; Remove unused Utils.hpp includes. #1269

refactor(clp-s): Delete unused clp_s::StringUtils functions; Remove unused Utils.hpp includes. #1269

Uh oh!

Conversation

gibber9809 commented Aug 27, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bill-hbrhbr left a comment

Choose a reason for hiding this comment

Uh oh!

gibber9809 commented Sep 3, 2025

Uh oh!

Bill-hbrhbr left a comment

Choose a reason for hiding this comment

Uh oh!

kirkrodrigues left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(clp-s): Delete unused `clp_s::StringUtils` functions; Remove unused `Utils.hpp` includes. #1269

refactor(clp-s): Delete unused `clp_s::StringUtils` functions; Remove unused `Utils.hpp` includes. #1269

gibber9809 commented Aug 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Aug 27, 2025 •

edited

Loading