refactor(core): Resolve clang-tidy violations in string_utils.#2003
Open
Nathan903 wants to merge 18 commits intoy-scope:mainfrom
Open
refactor(core): Resolve clang-tidy violations in string_utils.#2003Nathan903 wants to merge 18 commits intoy-scope:mainfrom
Nathan903 wants to merge 18 commits intoy-scope:mainfrom
Conversation
Contributor
WalkthroughModernizes C++ function signatures in string utilities by converting them to use auto with trailing return type syntax and adding [[nodiscard]] attributes to enforce value-usage checks. Test code is refactored to adopt snake_case naming conventions. Static analysis exclusions for these modules are removed to enable linting checks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Nathan903
changed the title
Bill-hbrhbr
requested changes
Feb 17, 2026
Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/core/src/clp/string_utils/string_utils.cpp (1)
144-148:⚠️ Potential issue | 🟡 Minor
to_lowerdefinition is inconsistent with the updated header declaration.The header (line 84) was updated in this PR to
auto to_lower(std::string& str) -> void;, but the definition here still uses the traditionalvoid to_lower(string& str)form. Every other function definition in this file uses the trailing-return-type style; this is the only outlier.♻️ Proposed fix
-void to_lower(string& str) { +auto to_lower(string& str) -> void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/string_utils/string_utils.cpp` around lines 144 - 148, The definition of to_lower uses the old-style signature but the header declares it with a trailing-return-type; update the function definition for to_lower(std::string& str) to use the same trailing-return-type form (auto to_lower(std::string& str) -> void) so it matches the header and other functions in string_utils.cpp; keep the body unchanged (std::transform with std::tolower) and ensure the parameter type matches the header's std::string reference and the function name to_lower is used exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp/string_utils/string_utils.cpp`:
- Around line 150-158: Remove the redundant docstring above the is_wildcard
function in string_utils.cpp because the canonical documentation belongs in
string_utils.hpp; delete the comment block shown and keep the implementation
as-is (auto is_wildcard(char c) -> bool { ... }), and if you prefer the more
precise phrasing ("true if '?' or '*'") migrate that wording into the header
comment for is_wildcard in string_utils.hpp instead of duplicating it in the
.cpp.
- Around line 264-266: The bug is that the flag is_escaped (declared alongside
char w and t) is never reset between iterations so a prior escape makes
subsequent '?' lose wildcard behavior; fix by making is_escaped a
block-local/iteration-local value (or explicitly reset it to false at the top of
the loop) so it accurately reflects whether the current w came from a '\' escape
before the mismatch check that uses is_escaped (see variables w, t, is_escaped
and the mismatch logic around the existing '?' handling), and add a unit test
case with wild="\\a?" and tame="ab" expecting true.
- Around line 32-37: The definition of advance_tame_to_next_match is missing the
[[nodiscard]] attribute present on its forward declaration; update the function
definition signature for advance_tame_to_next_match(char const*& tame_current,
char const*& tame_bookmark, char const* tame_end, char const*& wild_current) to
include [[nodiscard]] so it matches the declaration and the rest of the updated
functions.
In `@components/core/tests/test-string_utils.cpp`:
- Around line 409-410: Remove the duplicated assertions that call
wildcard_match_unsafe_case_sensitive with identical arguments: replace the
second occurrence of wildcard_match_unsafe_case_sensitive("ababac", "*abac*")
and the second occurrence of wildcard_match_unsafe_case_sensitive("abAbac",
"*Abac*") with either a removal or a distinct test case input to exercise
different behavior (e.g., different string or pattern) so each assertion adds
coverage rather than duplicating the same check.
- Around line 242-244: The GIVEN descriptions say the pattern starts/ends with
"?*" but the test uses "*?" due to copy-paste; update the test cases that call
wildcard_match_unsafe_case_sensitive so the wild_string matches the description:
change the first case's wild_string from "*?cd" to "?*cd" (GIVEN "Wild begins
with \"?*\"...") and change the later case's wild_string from "abc*?" to "abc?*"
(GIVEN "Wild ends with \"?*\"..."), leaving tame_string and the REQUIRE
assertions unchanged.
- Line 403: Replace negated expressions using the unary ! with the preferred
form "false == <expression>" throughout the test file: locate uses like
all_passed &= !wildcard_match_unsafe_case_sensitive(...); and change them to
all_passed &= (false == wildcard_match_unsafe_case_sensitive(...)); apply this
consistently for all occurrences (e.g., lines referencing
wildcard_match_unsafe_case_sensitive and any other function calls negated with
!) and keep the parentheses to preserve precedence around the comparison; ensure
you update every listed occurrence rather than adding new logical negation.
- Around line 385-391: The test contains a duplicate assertion using the same
tame_string/wild_string pair; update the second sub-case in the GIVEN block
("All lower case tame and mixed lower and upper case wild") so it uses the
intended mixed-case pattern (e.g., set wild_string to "A?C*") and keep the
REQUIRE calling wildcard_match_unsafe(tame_string, wild_string,
is_case_sensitive) to verify case-insensitive matching; modify the second
assignment of wild_string (and optionally tame_string) instead of duplicating
the first pair so the test covers the mixed upper-case 'C'.
---
Outside diff comments:
In `@components/core/src/clp/string_utils/string_utils.cpp`:
- Around line 144-148: The definition of to_lower uses the old-style signature
but the header declares it with a trailing-return-type; update the function
definition for to_lower(std::string& str) to use the same trailing-return-type
form (auto to_lower(std::string& str) -> void) so it matches the header and
other functions in string_utils.cpp; keep the body unchanged (std::transform
with std::tolower) and ensure the parameter type matches the header's
std::string reference and the function name to_lower is used exactly.
Comment on lines
+32
to
+37
| inline auto advance_tame_to_next_match( | ||
| char const*& tame_current, | ||
| char const*& tame_bookmark, | ||
| char const* tame_end, | ||
| char const*& wild_current | ||
| ) { | ||
| ) -> bool { |
Contributor
There was a problem hiding this comment.
[[nodiscard]] missing from the definition of advance_tame_to_next_match.
The forward declaration (line 25) correctly carries [[nodiscard]], but the definition at line 32 omits it. The PR explicitly adds [[nodiscard]] to both declarations and definitions for all other updated functions; this definition is the lone exception.
♻️ Proposed fix
-inline auto advance_tame_to_next_match(
+[[nodiscard]] inline auto advance_tame_to_next_match(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| inline auto advance_tame_to_next_match( | |
| char const*& tame_current, | |
| char const*& tame_bookmark, | |
| char const* tame_end, | |
| char const*& wild_current | |
| ) { | |
| ) -> bool { | |
| [[nodiscard]] inline auto advance_tame_to_next_match( | |
| char const*& tame_current, | |
| char const*& tame_bookmark, | |
| char const* tame_end, | |
| char const*& wild_current | |
| ) -> bool { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/core/src/clp/string_utils/string_utils.cpp` around lines 32 - 37,
The definition of advance_tame_to_next_match is missing the [[nodiscard]]
attribute present on its forward declaration; update the function definition
signature for advance_tame_to_next_match(char const*& tame_current, char const*&
tame_bookmark, char const* tame_end, char const*& wild_current) to include
[[nodiscard]] so it matches the declaration and the rest of the updated
functions.
Contributor
There was a problem hiding this comment.
No we only add [[nodiscard]] to declarations, not definitions.
Bill-hbrhbr
requested changes
Feb 18, 2026
|
|
||
| template <std::integral integer_t> | ||
| bool convert_string_to_int(std::string_view raw, integer_t& converted) { | ||
| [[nodiscard]] auto convert_string_to_int(std::string_view raw, integer_t& converted) -> bool { |
Contributor
There was a problem hiding this comment.
Suggested change
| [[nodiscard]] auto convert_string_to_int(std::string_view raw, integer_t& converted) -> bool { | |
| auto convert_string_to_int(std::string_view raw, integer_t& converted) -> bool { |
| char const* tame_bookmark{nullptr}; | ||
| char const* wild_bookmark{nullptr}; | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| char const* tame_end = tame_current + tame_length; |
Contributor
There was a problem hiding this comment.
Suggested change
| char const* tame_end = tame_current + tame_length; | |
| char const* tame_end{tame_current + tame_length}; |
| // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| char const* tame_end = tame_current + tame_length; | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| char const* wild_end = wild_current + wild_length; |
Contributor
There was a problem hiding this comment.
Suggested change
| char const* wild_end = wild_current + wild_length; | |
| char const* wild_end{wild_current + wild_length}; |
| // Handle escaped characters | ||
| if ('\\' == w) { | ||
| is_escaped = true; | ||
| bool is_escaped = '\\' == w; |
Contributor
There was a problem hiding this comment.
Suggested change
| bool is_escaped = '\\' == w; | |
| auto const is_escaped = ('\\' == w); |
or
Suggested change
| bool is_escaped = '\\' == w; | |
| bool const is_escaped{'\\' == w}; |
Comment on lines
+216
to
+217
| // We convert to lowercase (rather than uppercase) anticipating that | ||
| // callers use lowercase more frequently, so little will need to change. |
Contributor
There was a problem hiding this comment.
Suggested change
| // We convert to lowercase (rather than uppercase) anticipating that | |
| // callers use lowercase more frequently, so little will need to change. | |
| // We convert to lowercase (rather than uppercase) anticipating that callers | |
| // use lowercase more frequently, so little will need to change. |
Reflow comments for the 80 character line limit used in this file.
| std::string in, | ||
| std::string const& expected) { | ||
| replace_unescaped_char(escape_char, from_char, to_char, in); | ||
| REQUIRE((in == expected)); |
Contributor
There was a problem hiding this comment.
Suggested change
| REQUIRE(in == expected); |
Find all occurrences where REQUIRE wraps comparisons in double parentheses and remove the extra parentheses. After upgrading from Catch2 v2 to v3, the additional parentheses around comparison expressions are no longer required.
| } | ||
| ++c; | ||
| } | ||
| REQUIRE(unescaped_string == unescape_string(escaped_string)); |
Contributor
There was a problem hiding this comment.
Suggested change
| REQUIRE(unescape_string(escaped_string) == unescaped_string); |
| tameString = "abcd", wildString = "a*"; | ||
| REQUIRE(wildcard_match_unsafe_case_sensitive(tameString, wildString) == true); | ||
| tame_string = "abcd", wild_string = "a*"; | ||
| REQUIRE(wildcard_match_unsafe_case_sensitive(tame_string, wild_string) == true); |
Contributor
There was a problem hiding this comment.
Suggested change
| REQUIRE(wildcard_match_unsafe_case_sensitive(tame_string, wild_string) == true); | |
| REQUIRE(wildcard_match_unsafe_case_sensitive(tame_string, wild_string)); |
Let's remove all redundant ==true
| allPassed &= !wildcard_match_unsafe_case_sensitive("a12b12", "a12b"); | ||
| allPassed &= wildcard_match_unsafe_case_sensitive("a12b12", "*12*12*"); | ||
| REQUIRE(allPassed == true); | ||
| all_passed &= wildcard_match_unsafe_case_sensitive("abcccd", "*ccd"); |
Contributor
There was a problem hiding this comment.
After giving this some thought, I think there's no upside to report a single status for a bunch of tests. You cannot pinpoint the exact places where you fail.
Let's replace all passing function calls with REQUIRE(...) and all failing ones with REQUIRE_FALSE(...). Hopefully you can make use of AI tools to help you do this chore.
Comment on lines
+604
to
+609
| test_reps = n_reps; | ||
| t1 = high_resolution_clock::now(); | ||
| while (testReps--) { | ||
| BOOST_FOREACH (boost::tie(tameStr, wildStr), boost::combine(tameVec, wildVec)) { | ||
| allPassed_currentImplementation | ||
| &= wildcard_match_unsafe_case_sensitive(tameStr, wildStr); | ||
| while (test_reps-- != 0) { | ||
| BOOST_FOREACH (boost::tie(tame_str, wild_str), boost::combine(tame_vec, wild_vec)) { | ||
| all_passed_current_implementation | ||
| &= wildcard_match_unsafe_case_sensitive(tame_str, wild_str); |
Contributor
There was a problem hiding this comment.
I guess for this block it makes sense to use all passed variable due to the sheer number of iterations.
Bill-hbrhbr
reviewed
Feb 18, 2026
| ) { | ||
| ) -> bool { | ||
| auto w = *wild_current; | ||
| if ('?' != w) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if ('?' != w) { | |
| if (cSingleCharWildcard != w) { |
let's make use of the constants inside constants.hpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolve
clang-tidyviolations in thestring_utilslibrary (string_utils.hpp,string_utils.cpp) and its unit test (test-string_utils.cpp), and updatetaskfiles/lint.yamlto remove these files from the exclusion list.Checklist
Validation performed
components/coresuccessfully to ensure no compilation errors were introduced.components/core/build/tests/test-string_utilsto verify functionality matches previous behavior.clang-tidyon the modified files to verify no warnings remain.Summary by CodeRabbit
Release Notes
Refactor
Chores