fix(core-clp): Preserve escaped ?-wildcards in queries (fixes #243).#1070
Conversation
WalkthroughAdds clp::string_utils::replace_unescaped_char and wildcard/escape constants; removes those constants from regex_utils; updates regex translation and GrepCore to use the new constants and replace only unescaped '?' with '*'; adds unit tests for the new helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GrepCore
participant StringUtils
Caller->>GrepCore: process_raw_query(query)
GrepCore->>StringUtils: replace_unescaped_char(cWildcardEscapeChar, cSingleCharWildcard, cZeroOrMoreCharsWildcard, query)
StringUtils-->>GrepCore: (query modified in-place)
GrepCore->>StringUtils: clean_up_wildcard_search_string(query)
StringUtils-->>GrepCore: (query normalized)
GrepCore-->>Caller: processed query
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–30 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
quinntaylormitchell
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp/Grep.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/core/src/clp/Grep.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
components/core/src/clp/Grep.cpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (1)
components/core/src/clp/Grep.cpp (1)
539-540: LGTM! Well-implemented fix for escaped wildcards.The integration of the new function is clean and well-documented. The comment clearly explains the purpose, and the change preserves the existing logic flow while adding the required escape handling functionality.
…dded docstring; Rename replace_unescaped_wildcards() to replace_unescaped_char()
…rget_char as arguments; update docstring and call to replace_unescaped_char() accordingly; lint
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/tests/test-string_utils.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/core/tests/test-string_utils.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
components/core/tests/test-string_utils.cpp (11)
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Learnt from: LinZhihao-723
PR: y-scope/clp#557
File: components/core/tests/test-ir_encoding_methods.cpp:1216-1286
Timestamp: 2024-10-13T09:27:43.408Z
Learning: In the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp`, suppressing the `readability-function-cognitive-complexity` warning is acceptable due to the expansion of Catch2 macros in C++ tests, and such test cases may not have readability issues.
Learnt from: LinZhihao-723
PR: y-scope/clp#856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:258-266
Timestamp: 2025-04-26T02:21:22.021Z
Learning: In the clp::ffi::ir_stream::search namespace, the design principle is that callers are responsible for type checking, not the called functions. If control flow reaches a function, input types should already be validated by the caller.
Learnt from: davemarco
PR: y-scope/clp#700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.hpp:153-155
Timestamp: 2025-01-30T19:26:33.869Z
Learning: When working with constexpr strings (string literals with static storage duration), std::string_view is the preferred choice for member variables as it's more efficient and safe, avoiding unnecessary memory allocations.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:148-157
Timestamp: 2025-05-02T22:27:59.347Z
Learning: In the `QueryHandlerImpl.cpp` file, the `unique_projected_columns` set (using `std::string_view`) is intentionally designed to only check for duplications within the local scope of the `create_projected_columns_and_projection_map` function. The team decided this is an acceptable use of `std::string_view` in a container since the referenced strings remain valid throughout the function's execution.
Learnt from: gibber9809
PR: y-scope/clp#584
File: components/core/src/clp_s/SchemaTree.hpp:171-171
Timestamp: 2024-11-12T18:46:20.933Z
Learning: In `components/core/src/clp_s/SchemaTree.hpp`, it's acceptable to use `std::string_view` as keys in `m_node_map` because `SchemaNode`'s `m_key_name` remains valid even after move operations or reallocations, preventing dangling references.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-clp_s-end_to_end.cpp:141-142
Timestamp: 2024-11-29T22:51:00.355Z
Learning: In 'components/core/tests/test-clp_s-end_to_end.cpp', using `WIFEXITED` and `WEXITSTATUS` with the return value of `std::system` is acceptable for checking the exit status of system commands, as per the C++ documentation.
Learnt from: gibber9809
PR: y-scope/clp#584
File: components/core/src/clp_s/SchemaTree.hpp:91-94
Timestamp: 2024-11-12T18:47:03.828Z
Learning: In `components/core/src/clp_s/SchemaTree.hpp`, the `SchemaNode` class uses `std::unique_ptr<char[]> m_key_buf` and `std::string_view m_key_name` to ensure that references to `m_key_name` remain valid even after `SchemaNode` is move-constructed.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
⏰ 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: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
🔇 Additional comments (1)
components/core/tests/test-string_utils.cpp (1)
10-10: Good addition of the using statement.The using statement for
replace_unescaped_charis correctly added and follows the existing pattern in the file.
…ng ‘escaped’ variable and then passing by reference
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp/string_utils/string_utils.cpp(1 hunks)components/core/tests/test-string_utils.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/core/tests/test-string_utils.cppcomponents/core/src/clp/string_utils/string_utils.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
components/core/tests/test-string_utils.cpp (13)
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Learnt from: LinZhihao-723
PR: y-scope/clp#557
File: components/core/tests/test-ir_encoding_methods.cpp:1216-1286
Timestamp: 2024-10-13T09:27:43.408Z
Learning: In the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp`, suppressing the `readability-function-cognitive-complexity` warning is acceptable due to the expansion of Catch2 macros in C++ tests, and such test cases may not have readability issues.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Learnt from: LinZhihao-723
PR: y-scope/clp#593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-clp_s-end_to_end.cpp:141-142
Timestamp: 2024-11-29T22:51:00.355Z
Learning: In 'components/core/tests/test-clp_s-end_to_end.cpp', using `WIFEXITED` and `WEXITSTATUS` with the return value of `std::system` is acceptable for checking the exit status of system commands, as per the C++ documentation.
Learnt from: LinZhihao-723
PR: y-scope/clp#856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:258-266
Timestamp: 2025-04-26T02:21:22.021Z
Learning: In the clp::ffi::ir_stream::search namespace, the design principle is that callers are responsible for type checking, not the called functions. If control flow reaches a function, input types should already be validated by the caller.
Learnt from: davemarco
PR: y-scope/clp#700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.hpp:153-155
Timestamp: 2025-01-30T19:26:33.869Z
Learning: When working with constexpr strings (string literals with static storage duration), std::string_view is the preferred choice for member variables as it's more efficient and safe, avoiding unnecessary memory allocations.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:148-157
Timestamp: 2025-05-02T22:27:59.347Z
Learning: In the `QueryHandlerImpl.cpp` file, the `unique_projected_columns` set (using `std::string_view`) is intentionally designed to only check for duplications within the local scope of the `create_projected_columns_and_projection_map` function. The team decided this is an acceptable use of `std::string_view` in a container since the referenced strings remain valid throughout the function's execution.
Learnt from: gibber9809
PR: y-scope/clp#584
File: components/core/src/clp_s/SchemaTree.hpp:171-171
Timestamp: 2024-11-12T18:46:20.933Z
Learning: In `components/core/src/clp_s/SchemaTree.hpp`, it's acceptable to use `std::string_view` as keys in `m_node_map` because `SchemaNode`'s `m_key_name` remains valid even after move operations or reallocations, preventing dangling references.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: gibber9809
PR: y-scope/clp#584
File: components/core/src/clp_s/SchemaTree.hpp:91-94
Timestamp: 2024-11-12T18:47:03.828Z
Learning: In `components/core/src/clp_s/SchemaTree.hpp`, the `SchemaNode` class uses `std::unique_ptr<char[]> m_key_buf` and `std::string_view m_key_name` to ensure that references to `m_key_name` remain valid even after `SchemaNode` is move-constructed.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
🧬 Code Graph Analysis (1)
components/core/tests/test-string_utils.cpp (1)
components/core/src/clp/string_utils/string_utils.cpp (2)
replace_unescaped_char(132-147)replace_unescaped_char(133-133)
⏰ 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). (6)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (1)
components/core/tests/test-string_utils.cpp (1)
26-61: Excellent edge-case coverage addedThe new test vectors (empty input, dangling escapes, even/odd backslashes, mixed escaped/unescaped targets) exercise all tricky paths of
replace_unescaped_charand will quickly detect future regressions.
quinntaylormitchell
changed the title
…use those meta characters in the call to replace_unescaped_char()
quinntaylormitchell
changed the title
haiqi96
left a comment
haiqi96
left a comment
There was a problem hiding this comment.
The change looks good to me.
…est case; Address Kirk's other comments
haiqi96
left a comment
haiqi96
left a comment
There was a problem hiding this comment.
Reviewed the newly added changes and they look reasonable to me
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Looked at the PR with fresh eyes and spotted a few small things.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
components/core/src/clp/regex_utils/regex_translation_utils.cpp (1)
219-225: Use regex constants for input parsing (keep wildcard constants for output).In
dot_state_transition, you’re checking the next regex character againstcZeroOrMoreCharsWildcard(a wildcard constant). Functionally this is the same'*'today, but semantically it’s a regex input and should be checked with the regex constant. This avoids confusion and prevents issues if these ever diverge.Apply this diff:
- switch (*it) { - case cZeroOrMoreCharsWildcard: + switch (*it) { + case cRegexZeroOrMore: wildcard_str += cZeroOrMoreCharsWildcard; break; case cRegexOneOrMore: wildcard_str = wildcard_str + cSingleCharWildcard + cZeroOrMoreCharsWildcard; break;If you want, I can scan the codebase to ensure no other regex input checks are using wildcard constants by mistake.
♻️ Duplicate comments (1)
components/core/src/clp/string_utils/constants.hpp (1)
1-12: Optional: Add#pragma oncealongside include guards.Include guards are correct; adding
#pragma oncecan provide a minor build-speed optimisation and future-proof against guard typos. This was raised previously.+#pragma once #ifndef CLP_STRING_UTILS_CONSTANTS_HPP #define CLP_STRING_UTILS_CONSTANTS_HPP
📜 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.
📒 Files selected for processing (6)
components/core/src/clp/GrepCore.hpp(2 hunks)components/core/src/clp/regex_utils/regex_translation_utils.cpp(6 hunks)components/core/src/clp/string_utils/constants.hpp(1 hunks)components/core/src/clp/string_utils/string_utils.cpp(1 hunks)components/core/src/clp/string_utils/string_utils.hpp(1 hunks)components/core/tests/test-string_utils.cpp(2 hunks)
🧰 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/string_utils/constants.hppcomponents/core/src/clp/GrepCore.hppcomponents/core/src/clp/string_utils/string_utils.cppcomponents/core/src/clp/regex_utils/regex_translation_utils.cppcomponents/core/src/clp/string_utils/string_utils.hppcomponents/core/tests/test-string_utils.cpp
🧠 Learnings (3)
📚 Learning: 2025-07-01T14:49:07.333Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/src/clp/string_utils/constants.hpp
📚 Learning: 2024-10-24T14:45:26.265Z
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
Applied to files:
components/core/src/clp/string_utils/string_utils.cpp
📚 Learning: 2025-01-30T19:26:33.869Z
Learnt from: davemarco
PR: y-scope/clp#700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.hpp:153-155
Timestamp: 2025-01-30T19:26:33.869Z
Learning: When working with constexpr strings (string literals with static storage duration), std::string_view is the preferred choice for member variables as it's more efficient and safe, avoiding unnecessary memory allocations.
Applied to files:
components/core/src/clp/regex_utils/regex_translation_utils.cpp
🧬 Code Graph Analysis (2)
components/core/src/clp/GrepCore.hpp (2)
components/core/src/clp/string_utils/string_utils.hpp (1)
replace_unescaped_char(69-70)components/core/src/clp/string_utils/string_utils.cpp (2)
replace_unescaped_char(116-136)replace_unescaped_char(116-121)
components/core/tests/test-string_utils.cpp (2)
components/core/src/clp/string_utils/string_utils.hpp (5)
clean_up_wildcard_search_string(88-88)convert_string_to_int(140-140)convert_string_to_int(143-149)convert_string_to_int(143-143)replace_unescaped_char(69-70)components/core/src/clp/string_utils/string_utils.cpp (4)
clean_up_wildcard_search_string(154-189)clean_up_wildcard_search_string(154-154)replace_unescaped_char(116-136)replace_unescaped_char(116-121)
⏰ 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). (7)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (8)
components/core/src/clp/regex_utils/regex_translation_utils.cpp (2)
16-19: Centralizing wildcard/escape constants looks good.Using
string_utils::cWildcardEscapeChar,cSingleCharWildcard, andcZeroOrMoreCharsWildcardkeeps these meta characters defined in one place and removes duplication across modules.
351-354: Escaping wildcard meta characters uses the centralized escape char correctly.Appending
cWildcardEscapeCharbefore meta characters maintains consistency with the new constants header.components/core/src/clp/string_utils/string_utils.cpp (1)
116-136: LGTM — predicate flow reads clearly and handles odd/even escapes.The order: handled-escaped -> handle-escape-char -> check-from_char, makes the intent explicit and avoids subtle branching errors. Good use of brace-init for
escaped.components/core/src/clp/GrepCore.hpp (1)
166-174: Correct fix: replace only unescaped '?' with '*' in heuristic processing.Switching from a blanket
std::replacetostring_utils::replace_unescaped_charpreserves escaped wildcards (\?) while still enabling sub-query generation that expects*. The follow-up cleanup step keeps the wildcard string normalized. Nice.components/core/tests/test-string_utils.cpp (4)
6-6: Centralized constants include is appropriateIncluding string_utils/constants.hpp here is correct and aligns tests with the new source of truth for wildcard/escape chars.
30-39: Clean helper for exercising replace_unescaped_charPassing the input string by value and asserting equality on the mutated copy is a neat, side-effect-free pattern. The assertion form REQUIRE((in == expected)); matches our style.
73-74: Correct use of new string_utils constantsGood call using cWildcardEscapeChar, cSingleCharWildcard, and cZeroOrMoreCharsWildcard in the canonical test section, ensuring tests stay aligned with production semantics.
76-78: Custom escape/source/target scenario covers non-conventional usageSolid check that the algorithm behaves with non-default characters and that escapes only protect the immediate next char.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/core/src/clp/string_utils/string_utils.hpp (2)
69-74: Signature, naming, and placement look good.
- Trailing return type and const-qualified by-value chars are consistent with the implementation and recent standards.
- Parameter names (escape_char/from_char/to_char/str) are clear.
- The function is placed immediately after replace_characters as requested earlier.
61-63: State this as a precondition (and enforce it in .cpp).Replace the “NOTE … undefined behaviour” with an explicit precondition; this avoids hand-wavy UB in docs and clarifies caller responsibility. Also ensure the .cpp enforces it with a debug assertion.
- * NOTE: `from_char` and `escape_char` must not be the same character. If they are, the function's - * behaviour is undefined. + * Precondition: `escape_char` != `from_char`.If not already in the implementation, add:
assert(escape_char != from_char && "escape_char and from_char must differ");
📜 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.
📒 Files selected for processing (1)
components/core/src/clp/string_utils/string_utils.hpp(1 hunks)
🧰 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/string_utils/string_utils.hpp
⏰ 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: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
components/core/src/clp/string_utils/string_utils.hpp (2)
58-60: Doc summary reads well.“Replace unescaped instances of from_char with to_char in str.” is concise and accurate.
69-74: replace_unescaped_char complies with boolean-negation guideline
I searched the implementation incomponents/core/src/clp/string_utils/string_utils.cppand found no occurrences of!escapedornot escaped. The code already avoids using!exprand meets our guideline.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
components/core/src/clp/regex_utils/regex_translation_utils.cpp (2)
218-226: Use the regex quantifier constant in the switch (not the wildcard constant).Line 219 compares against cZeroOrMoreCharsWildcard while scanning a regex pattern. Semantically this should use cRegexZeroOrMore for the quantifier check, and only use cZeroOrMoreCharsWildcard when emitting the wildcard result. This keeps domains separated and prevents accidental mismatches if constants diverge.
Apply this diff:
switch (*it) { - case cZeroOrMoreCharsWildcard: + case cRegexZeroOrMore: wildcard_str += cZeroOrMoreCharsWildcard; break;
350-355: Escape wildcard metacharacters with cWildcardEscapeChar, not cEscapeChar.append_char_to_wildcard is building a wildcard string, so it should use the wildcard escape constant, not the regex escape constant. This aligns with the separation introduced in string_utils and avoids cross-domain coupling.
Apply this diff:
auto append_char_to_wildcard(char ch, string& wildcard_str) -> void { if (cWildcardMetaCharsLut.at(ch)) { - wildcard_str += cEscapeChar; + wildcard_str += cWildcardEscapeChar; } wildcard_str += ch; }
♻️ Duplicate comments (3)
components/core/src/clp/string_utils/string_utils.hpp (3)
61-62: State a precondition instead of “undefined behaviour” and enforce it in the .cpp.Replace the note with a firm precondition. Also add a debug assertion in the implementation to catch misuse.
Apply this diff to the comment:
- * NOTE: `from_char` and `escape_char` must not be the same character. If they are, the function's - * behaviour is undefined. + * Precondition: `escape_char != from_char`.And in components/core/src/clp/string_utils/string_utils.cpp, add:
#include <cassert> // ... auto replace_unescaped_char(char const escape_char, char const from_char, char const to_char, std::string& str) noexcept -> void { assert(escape_char != from_char && "escape_char and from_char must differ"); // existing implementation... }
64-67: Clarify @param documentation (what gets replaced by what).Make parameter roles explicit and punctuate consistently.
Apply this diff:
- * @param escape_char The character used for escaping - * @param from_char - * @param to_char - * @param str String in which to replace the characters + * @param escape_char The character used for escaping. + * @param from_char Character to search for (only unescaped occurrences are replaced). + * @param to_char Replacement character. + * @param str String in which to replace the characters.
69-70: Mark the API noexcept and align const qualifiers with the definition.This strengthens the contract (no-throw) and keeps declaration/definition consistent. The operation is in-place and non-allocating, so noexcept is appropriate.
Apply this diff:
-auto replace_unescaped_char(char escape_char, char from_char, char to_char, std::string& str) +auto replace_unescaped_char(char const escape_char, char const from_char, char const to_char, std::string& str) noexcept -> void;Remember to mirror noexcept (and the same const qualifiers) in the .cpp definition if not already present.
📜 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.
📒 Files selected for processing (3)
components/core/src/clp/regex_utils/constants.hpp(0 hunks)components/core/src/clp/regex_utils/regex_translation_utils.cpp(2 hunks)components/core/src/clp/string_utils/string_utils.hpp(1 hunks)
💤 Files with no reviewable changes (1)
- components/core/src/clp/regex_utils/constants.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/string_utils/string_utils.hppcomponents/core/src/clp/regex_utils/regex_translation_utils.cpp
🧠 Learnings (1)
📚 Learning: 2025-01-30T19:26:33.869Z
Learnt from: davemarco
PR: y-scope/clp#700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.hpp:153-155
Timestamp: 2025-01-30T19:26:33.869Z
Learning: When working with constexpr strings (string literals with static storage duration), std::string_view is the preferred choice for member variables as it's more efficient and safe, avoiding unnecessary memory allocations.
Applied to files:
components/core/src/clp/regex_utils/regex_translation_utils.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). (5)
- 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
🔇 Additional comments (2)
components/core/src/clp/string_utils/string_utils.hpp (1)
58-60: Good addition and placement.Clear, concise summary. Placement right after replace_characters aligns with earlier feedback and logical grouping.
components/core/src/clp/regex_utils/regex_translation_utils.cpp (1)
8-9: Include of string_utils/constants.hpp is appropriate and consistent.Good call to include the shared constants header from string_utils; it centralises wildcard-related definitions and prevents duplication.
| using clp::string_utils::cSingleCharWildcard; | ||
| using clp::string_utils::cZeroOrMoreCharsWildcard; | ||
| using clp::string_utils::is_alphabet; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also import cWildcardEscapeChar for wildcard escaping.
You already import wildcard constants. Since this file emits wildcard escapes (see append_char_to_wildcard), import the wildcard escape constant as well to avoid coupling to regex escape semantics.
Apply this diff to add the import:
using clp::string_utils::cSingleCharWildcard;
using clp::string_utils::cZeroOrMoreCharsWildcard;
+using clp::string_utils::cWildcardEscapeChar;
using clp::string_utils::is_alphabet;📝 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.
| using clp::string_utils::cSingleCharWildcard; | |
| using clp::string_utils::cZeroOrMoreCharsWildcard; | |
| using clp::string_utils::is_alphabet; | |
| using clp::string_utils::cSingleCharWildcard; | |
| using clp::string_utils::cZeroOrMoreCharsWildcard; | |
| using clp::string_utils::cWildcardEscapeChar; | |
| using clp::string_utils::is_alphabet; |
🤖 Prompt for AI Agents
In components/core/src/clp/regex_utils/regex_translation_utils.cpp around lines
17 to 19, add an import for the wildcard escape constant: bring in
clp::string_utils::cWildcardEscapeChar alongside the existing
cSingleCharWildcard, cZeroOrMoreCharsWildcard, and is_alphabet; update the using
declarations so append_char_to_wildcard and any other functions emit wildcard
escapes using cWildcardEscapeChar instead of relying on regex escape semantics.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
fix(core-clp): Preserve escaped `?`-wildcards in queries (fixes #243).
|
clp-core-build-macos failures are unrelated and due to #1218. |
quinntaylormitchell
changed the title
?-wildcards in queries (fixes #243).
…pe#243). (y-scope#1070) Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com> Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
3 participants
Description
Issue #243 describes a bug wherein queries containing the character sequence
\?were not being matched with the appropriate search results.?wildcard characters are replaced with*during search, but this was being done indiscriminately, i.e.?was being replaced with*regardless of whether or not it was escaped. This PR adds a helper function tocomponents/core/src/clp/string_utils/string_utils.cppcalledreplace_unescaped_char(), which replaces the call tostd::replace()that was being performed inGrepCore::process_raw_query()replace_unescaped_char()callsstd::replace_if()for the string passed into it. The predicate function passed intostd::replace_if()is a lambda function which returnstrueif the current character is the target character AND the current character is not escaped. A boolean calledescapedalternates betweenfalseandtrueupon encountering multiple escape characters in a row, and defaults to false upon encountering a non-escape character.std::replace_if()has the same complexity asstd::replace(), and it is single-run-through. The predicate (lambda) function does not require any caching.NOTE: This PR does not fix the bug that affects queries that contain the character sequence
\\?; this bug is described in issue #427 and fixed in PR #428.In addition, added unit tests to
test-string_utils.cppto test the newreplace_unescaped_char()function.Checklist
breaking change.
Validation performed
test-string_utils.cpp.Summary by CodeRabbit
Improvements
Bug Fixes
Tests