-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clp: Add missing C++ standard library includes in IR parsing files. #561
Conversation
WalkthroughThe changes involve modifications to three files within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser
participant LogType
User->>Parser: Call append_constant_to_logtype(constant, escape_handler, logtype)
Parser->>Parser: Iterate over characters in constant
alt Escape character found
Parser->>LogType: Append substring to logtype
Parser->>escape_handler: Call escape handler
else Variable placeholder found
Parser->>LogType: Append substring to logtype
Parser->>escape_handler: Call escape handler
end
Parser->>LogType: Append remaining substring to logtype
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/core/src/clp/ir/parsing.inc (1)
Line range hint
12-35
: Consider adding documentation and optimizations.A few suggestions to improve the implementation:
- Document the EscapeHandler concept requirements
- Pre-reserve space in logtype
- Simplify the boolean expression
Here's a suggested improvement:
template <typename EscapeHandler> +// EscapeHandler requirements: +// - Callable with signature: void(std::string_view, size_t&, std::string&) +// - May modify the size_t index for multi-character escape sequences void append_constant_to_logtype( std::string_view constant, EscapeHandler escape_handler, std::string& logtype ) { size_t begin_pos = 0; auto constant_len = constant.length(); + logtype.reserve(logtype.size() + constant_len); // Reserve space upfront for (size_t i = 0; i < constant_len; ++i) { auto const c = constant[i]; - bool const is_escape_char = (enum_to_underlying_type(VariablePlaceholder::Escape) == c); - if (false == is_escape_char && false == is_variable_placeholder(c)) { + if (c != enum_to_underlying_type(VariablePlaceholder::Escape) + && !is_variable_placeholder(c)) { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/core/src/clp/ir/parsing.cpp (1 hunks)
- components/core/src/clp/ir/parsing.hpp (1 hunks)
- components/core/src/clp/ir/parsing.inc (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- components/core/src/clp/ir/parsing.cpp
- components/core/src/clp/ir/parsing.hpp
🧰 Additional context used
🔇 Additional comments (2)
components/core/src/clp/ir/parsing.inc (2)
4-6
: LGTM: Header additions address the Windows build issue.The added standard library headers are necessary and correctly ordered according to C++ best practices.
Line range hint
28-30
: Verify escape handler bounds safety.The escape_handler can modify 'i', but there's no verification that it stays within bounds. Consider adding bounds checking or documenting this requirement.
Co-authored-by: kirkrodrigues <[email protected]>
* ffi: Add support for serializing/deserializing auto-generated and user-generated schema tree node IDs. (y-scope#557) Co-authored-by: kirkrodrigues <[email protected]> * clp: Add missing C++ standard library includes in IR parsing files. (y-scope#561) Co-authored-by: kirkrodrigues <[email protected]> * log-viewer-webui: Update `yscope-log-viewer` to the latest version (which uses `clp-ffi-js`). (y-scope#562) * package: Upgrade dependencies to resolve security issues. (y-scope#536) * clp-s: Implement table packing (y-scope#466) Co-authored-by: wraymo <[email protected]> Co-authored-by: Kirk Rodrigues <[email protected]> Co-authored-by: wraymo <[email protected]> * log-viewer-webui: Update `yscope-log-viewer` to the latest version. (y-scope#565) * ci: Switch GitHub macOS build workflow to use macos-13 (x86) and macos-14 (ARM) runners. (y-scope#566) * core: Add support for user-defined HTTP headers in `NetworkReader`. (y-scope#568) Co-authored-by: Lin Zhihao <[email protected]> Co-authored-by: Xiaochong Wei <[email protected]> * chore: Update to the latest version of yscope-dev-utils. (y-scope#574) * build(core): Upgrade msgpack to v7.0.0. (y-scope#575) * feat(ffi): Update IR stream protocol version handling in preparation for releasing the kv-pair IR stream format: (y-scope#573) - Bump the IR stream protocol version to 0.1.0 for the kv-pair IR stream format. - Treat the previous IR stream format's versions as backwards compatible. - Differentiate between backwards-compatible and supported versions during validation. Co-authored-by: kirkrodrigues <[email protected]> * fix(taskfiles): Trim trailing slash from URL prefix in `download-and-extract-tar` (fixes y-scope#577). (y-scope#578) * fix(ffi): Correct `clp::ffi::ir_stream::Deserializer::deserialize_next_ir_unit`'s return value when failing to read the next IR unit's type tag. (y-scope#579) * fix(taskfiles): Update `yscope-log-viewer` sources in `log-viewer-webui-clients` sources list (fixes y-scope#576). (y-scope#580) * fix(cmake): Add Homebrew path detection for `mariadb-connector-c` to fix macOS build failure. (y-scope#582) Co-authored-by: kirkrodrigues <[email protected]> * refactor(ffi): Make `get_schema_subtree_bitmap` a public method of `KeyValuePairLogEvent`. (y-scope#581) * ci: Schedule GitHub workflows to daily run to detect failures due to upgraded dependencies or environments. (y-scope#583) * docs: Update the required version of task. (y-scope#567) * Add pr check workflow --------- Co-authored-by: kirkrodrigues <[email protected]> Co-authored-by: Junhao Liao <[email protected]> Co-authored-by: Henry8192 <[email protected]> Co-authored-by: Devin Gibson <[email protected]> Co-authored-by: wraymo <[email protected]> Co-authored-by: wraymo <[email protected]> Co-authored-by: Xiaochong(Eddy) Wei <[email protected]> Co-authored-by: Xiaochong Wei <[email protected]> Co-authored-by: haiqi96 <[email protected]>
…-scope#561) Co-authored-by: kirkrodrigues <[email protected]>
Description
clp-ffi-py workflow build failed on Windows: y-scope/clp-ffi-py#70
This is due to missing
<string>
headers inclp/ir/parsing.cpp
andclp/ir/parsing.hpp
. This PR adds all missing headers to these files to solve the issue.Validation performed
Summary by CodeRabbit