Skip to content

Conversation

@davidlion
Copy link
Member

@davidlion davidlion commented Jan 13, 2026

Description

Although it is possible to get all of the information of a capture group match, it requires extensive internal knowledge and leveraging public fields (that should be made private in the future). This PR adds a high-level API to get the information in a clean and organized way. This removes the need to duplicate the code in any application trying to inspect the capture group matches.

The API is limited to not support repeating matches as they are known to not be working correctly.

This PR also updated LogEventView::get_logtype to correctly only substitute leaf capture group matches. Previously this method had weird/broken behaviour.

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

Added checks using the API for existing unit tests and added a unit test with nested capture groups.

Summary by CodeRabbit

  • New Features

    • Added public APIs to query capture matches and capture positions, plus explicit error codes for log-event handling.
  • Improvements

    • Streamlined capture-group processing in parsing with clearer error-aware results and safer value handling.
    • Optional integration with a new error-handling library in build/config.
  • Documentation

    • Requirements updated to reference the new library.
  • Tests

    • Tests updated to validate capture-match structures and parsing behaviour.
  • Chores

    • Formatting and build-task updates.

✏️ Tip: You can customize this high-level summary in your review settings.

@davidlion davidlion requested a review from a team as a code owner January 13, 2026 09:42
@coderabbitai
Copy link

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Walkthrough

Adds ystdlib error-handling/result APIs and capture-match types to LogEvent/Token, refactors capture processing to use these helpers, and updates build, config, docs, formatting, taskfiles, and tests to declare and install the ystdlib (and Boost) dependency.

Changes

Cohort / File(s) Summary
Build & Packaging
CMakeLists.txt, cmake/log_surgeon-config.cmake.in, taskfiles/deps.yaml, README.md
Adds ystdlib (>= 0.1.0) dependency; links ystdlib::error_handling PUBLIC to log_surgeon; conditionally registers ystdlib in package config; adds install-boost and install-ystdlib tasks; documents dependency in README.
Public API / Types
src/log_surgeon/LogEvent.hpp, src/log_surgeon/Token.hpp
Adds LogEventView::get_capture_matches() and get_capture_position() returning ystdlib::error_handling::Result; introduces LogEventErrorCodeEnum and error-code plumbing; adds Token::position_t, Token::CaptureMatchPosition, and Token::CaptureMatch; marks get_logtype() [[nodiscard]].
Implementation
src/log_surgeon/LogEvent.cpp
Implements capture helpers using ystdlib::error_handling::Result; refactors capture processing to use those helpers; adds LogEventErrorCategory and new error codes/messages.
Formatting / Headers
src/.clang-format
Updates external include regex to include ystdlib (`^<(fmt
Tests
tests/test-buffer-parser.cpp
Replaces capture expectation types with ExpectedCaptureMatch (includes leaf flag); adds <algorithm> include; removes using std::pair.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main changes: adding a new API for retrieving capture group matches and updating the get_logtype method to use it for leaf matches only.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c3acb2e and 0641f76.

📒 Files selected for processing (1)
  • src/log_surgeon/LogEvent.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

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

Files:

  • src/log_surgeon/LogEvent.cpp
🧠 Learnings (13)
📓 Common learnings
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:...
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
📚 Learning: 2025-10-22T15:40:29.992Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:...

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-12-03T16:38:15.354Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 199
File: src/log_surgeon/LogParser.cpp:154-155
Timestamp: 2025-12-03T16:38:15.354Z
Learning: In `src/log_surgeon/LogParser.cpp` and related parsing code, when `get_reg_ids_from_capture(capture)` returns an optional, calling `.value()` directly without checking is intentional for captures that should always have register IDs (e.g., `"timestamp"` captures). If the optional is nullopt, it indicates an internal programming error/invariant violation, and throwing `std::bad_optional_access` is the desired behavior rather than defensive error handling with continue/return.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-05-01T14:47:57.016Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-11-06T11:16:52.917Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 184
File: tests/test-reader-parser.cpp:150-154
Timestamp: 2025-11-06T11:16:52.917Z
Learning: In Catch2 test files for the y-scope/log-surgeon repository, an explicit `if (false == optional.has_value()) { return; }` check may be required after `REQUIRE(optional.has_value())` to prevent clang-tidy errors, even though the code is unreachable at runtime. clang-tidy's static analyzer doesn't recognize that REQUIRE aborts execution and may flag unsafe optional access without the explicit check.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-08-15T00:13:46.717Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 150
File: tests/test-wildcard-expression.cpp:0-0
Timestamp: 2025-08-15T00:13:46.717Z
Learning: In src/log_surgeon/wildcard_query_parser/WildcardExpression.cpp, the escaping logic treats consecutive backslashes as escape-literal pairs, not as individual escape characters. For input like R"(a\*b\?c\\)", position 7 is the escape character and position 8 is the literal backslash being escaped, so only position 7 should have is_escape() return true.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-08-08T10:22:26.739Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 144
File: src/log_surgeon/wildcard_query_parser/QueryInterpretation.cpp:98-120
Timestamp: 2025-08-08T10:22:26.739Z
Learning: In C++ file src/log_surgeon/wildcard_query_parser/QueryInterpretation.cpp, for QueryInterpretation::serialize(), prefer explicit std::holds_alternative/std::get branching over std::visit for readability; do not suggest refactoring to std::visit in future reviews.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-05-05T14:55:34.455Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lalr1Parser.tpp:661-665
Timestamp: 2025-05-05T14:55:34.455Z
Learning: The log-surgeon codebase follows a design approach where function contracts (like `ErrorCode::Success` guaranteeing a valid token) are trusted, and contract violations are allowed to throw exceptions rather than being explicitly checked at every call site.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-08-26T10:13:00.368Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 152
File: tests/test-query.cpp:61-67
Timestamp: 2025-08-26T10:13:00.368Z
Learning: In Catch2 unit tests, the REQUIRE macro already provides detailed debugging output when container comparisons (like std::set equality) fail, showing both expected and actual values. Additional CAPTURE statements are typically unnecessary for such comparisons.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2024-11-27T21:56:13.425Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T21:56:13.425Z
Learning: In the `log_surgeon` project, header guards in C++ header files should include `_HPP` at the end to match the filename. For example, in `RegisterHandler.hpp`, the header guard should be `LOG_SURGEON_FINITE_AUTOMATA_REGISTER_HANDLER_HPP`.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2024-11-27T22:25:35.608Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T22:25:35.608Z
Learning: In the `RegisterHandler` class in `src/log_surgeon/finite_automata/RegisterHandler.hpp`, the methods `add_register` and `append_position` rely on `emplace_back` and `m_prefix_tree.insert` to handle exceptions correctly and maintain consistent state without requiring additional exception handling.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-08-26T10:06:22.914Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 152
File: src/log_surgeon/wildcard_query_parser/Query.hpp:9-11
Timestamp: 2025-08-26T10:06:22.914Z
Learning: In y-scope/log-surgeon project, it's acceptable to include headers like log_surgeon/Lexer.hpp directly in header files rather than using forward declarations, even when only references are used in the interface. The project prefers the simplicity of direct includes over header coupling optimization through forward declarations.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-08-15T01:04:03.681Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 150
File: src/log_surgeon/wildcard_query_parser/ExpressionView.cpp:11-11
Timestamp: 2025-08-15T01:04:03.681Z
Learning: In y-scope/log-surgeon C++ codebase, prefer explicit includes in implementation files (.cpp) for types that are directly used, even when those headers are already transitively included through the corresponding header file. This makes dependencies explicit and improves maintainability.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
🧬 Code graph analysis (1)
src/log_surgeon/LogEvent.cpp (2)
src/log_surgeon/Lexer.hpp (5)
  • rule_id (66-67)
  • rule_id (74-75)
  • rule_id (176-177)
  • capture (189-191)
  • capture (219-220)
src/log_surgeon/LogEvent.hpp (2)
  • root_var (138-141)
  • root_var (156-157)
🪛 Cppcheck (2.19.0)
src/log_surgeon/LogEvent.cpp

[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 3-3: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 3-3: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[style] 73-73: The function 'get_mutable_active_buffer' is never used.

(unusedFunction)


[style] 76-76: The function 'add_token_group' is never used.

(unusedFunction)


[style] 84-84: The function 'add_token_chain' is never used.

(unusedFunction)


[style] 110-110: The function 'add_production' is never used.

(unusedFunction)


[style] 154-154: The function 'get_has_delimiters' is never used.

(unusedFunction)


[style] 156-156: The function 'get_delim_table' is never used.

(unusedFunction)


[style] 160-160: The function 'is_delimiter' is never used.

(unusedFunction)


[style] 164-164: The function 'get_dfa' is never used.

(unusedFunction)


[style] 86-86: The function 'set_input_buffer' is never used.

(unusedFunction)


[style] 94-94: The function 'get_input_pos' is never used.

(unusedFunction)


[style] 102-102: The function 'read_into_input' is never used.

(unusedFunction)


[style] 115-115: The function 'reset_log_event_view' is never used.

(unusedFunction)


[style] 120-120: The function 'get_log_event_view' is never used.

(unusedFunction)


[style] 68-68: The function 'add_delimiter' is never used.

(unusedFunction)


[style] 90-90: The function 'get_special_regex_characters' is never used.

(unusedFunction)


[style] 94-94: The function 'get_first_char' is never used.

(unusedFunction)


[style] 131-131: The function 'get_buffer_size' is never used.

(unusedFunction)


[style] 142-142: The function 'set_type_ids' is never used.

(unusedFunction)


[style] 125-125: The function 'get_tree_transitions' is never used.

(unusedFunction)


[style] 74-74: The function 'get_parent_id_unsafe' is never used.

(unusedFunction)


[style] 81-81: The function 'get_position' is never used.

(unusedFunction)


[style] 73-73: The function 'token_cast' is never used.

(unusedFunction)


[style] 81-81: The function 'non_terminal_cast' is never used.

(unusedFunction)


[style] 85-85: The function 'get_parser_ast' is never used.

(unusedFunction)


[style] 97-97: The function 'release_parser_ast' is never used.

(unusedFunction)


[style] 220-220: The function 'scan_with_wildcard' is never used.

(unusedFunction)

🔇 Additional comments (5)
src/log_surgeon/LogEvent.cpp (5)

58-102: LGTM! The refactored get_logtype correctly processes only leaf capture group matches.

The implementation properly:

  • Falls back to a simple variable tag when no captures exist
  • Iterates through ordered matches and only substitutes leaf captures
  • Preserves the surrounding token segments between capture boundaries

Based on learnings, this aligns with the two independent mechanisms for adding tags to the logtype string.


113-118: Clear comparator for ordering captures.

The comparator correctly orders matches by start position ascending, with ties broken by end position descending. This ensures outer (parent) captures come before inner (child) captures when they share the same start, which is essential for the leaf detection logic that follows.


122-127: Intentional specific error handling for future-proofing.

The explicit check for NoCaptureGroupMatch before continuing, with other errors propagating via line 126, is a deliberate design choice as discussed. This ensures that if new error codes are added to get_capture_position in the future, they will properly propagate rather than being silently ignored.


149-164: LGTM! Clean implementation of capture position retrieval.

The method correctly:

  • Retrieves register IDs for the capture
  • Gets reversed register positions for both start and end
  • Validates that positions exist and are non-negative before returning
  • Returns appropriate error code when no match exists

Based on learnings, this follows the preferred Result<T, ErrorCode> pattern from ystdlib-cpp.


216-231: LGTM! Error category specializations follow ystdlib conventions.

The name() and message() template specializations correctly implement the error category interface. The default case in the switch statement provides a safe fallback for any future error codes.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @README.md:
- Line 90: The README's dependency list incorrectly states "fmt >= 8.0.1" while
CMakeLists.txt requires fmt 11.2.0; update the README entry to reflect the
actual required version (e.g., "* [fmt] >= 11.2.0") so the documented
requirement matches the CMakeLists.txt configuration and prevents users from
using an incompatible fmt version.

In @src/log_surgeon/LogEvent.hpp:
- Around line 125-137: The docstring for get_capture_position incorrectly
references ClpsErrorCodeEnum::Failure; update the return/error section to
mention the actual error used (LogEventErrorCodeEnum::NoCaptureGroupMatch) and
any relevant wording about invalid capture positions; locate the
get_capture_position declaration (Token::CaptureMatchPosition return type,
parameters Token const& root_var and finite_automata::Capture const* const&
capture) and replace the enum reference and description accordingly so the
comment matches the implementation.

In @taskfiles/deps.yaml:
- Around line 97-114: The docs claim ystdlib-cpp >= 0.1.0 but
taskfiles/deps.yaml's install-ystdlib task pins an untagged commit via
TAR_URL/TAR_SHA256 (TAR_URL
"https://github.com/y-scope/ystdlib-cpp/archive/c03806a.tar.gz"); either update
README and CMakeLists.txt to state the repository is pinned to commit c03806a,
or replace the TAR_URL/TAR_SHA256 in the install-ystdlib task with the canonical
release tag and its matching checksum if a v0.1.0 (or other) release exists;
ensure CMAKE_PACKAGE_NAME "ystdlib" and CMAKE_GEN_ARGS remain consistent and
update any version strings in documentation to match the chosen pin.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09f8a25 and b7375f2.

📒 Files selected for processing (9)
  • CMakeLists.txt
  • README.md
  • cmake/log_surgeon-config.cmake.in
  • src/.clang-format
  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/Token.hpp
  • taskfiles/deps.yaml
  • tests/test-buffer-parser.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

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

Files:

  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • tests/test-buffer-parser.cpp
  • src/log_surgeon/Token.hpp
🧠 Learnings (23)
📓 Common learnings
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:...
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lalr1Parser.tpp:661-665
Timestamp: 2025-05-05T14:55:34.455Z
Learning: The log-surgeon codebase follows a design approach where function contracts (like `ErrorCode::Success` guaranteeing a valid token) are trusted, and contract violations are allowed to throw exceptions rather than being explicitly checked at every call site.
📚 Learning: 2025-08-15T01:04:03.681Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 150
File: src/log_surgeon/wildcard_query_parser/ExpressionView.cpp:11-11
Timestamp: 2025-08-15T01:04:03.681Z
Learning: In y-scope/log-surgeon C++ codebase, prefer explicit includes in implementation files (.cpp) for types that are directly used, even when those headers are already transitively included through the corresponding header file. This makes dependencies explicit and improves maintainability.

Applied to files:

  • CMakeLists.txt
  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • cmake/log_surgeon-config.cmake.in
  • README.md
📚 Learning: 2025-05-01T14:47:57.016Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.

Applied to files:

  • CMakeLists.txt
  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • tests/test-buffer-parser.cpp
📚 Learning: 2025-08-26T10:06:22.914Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 152
File: src/log_surgeon/wildcard_query_parser/Query.hpp:9-11
Timestamp: 2025-08-26T10:06:22.914Z
Learning: In y-scope/log-surgeon project, it's acceptable to include headers like log_surgeon/Lexer.hpp directly in header files rather than using forward declarations, even when only references are used in the interface. The project prefers the simplicity of direct includes over header coupling optimization through forward declarations.

Applied to files:

  • CMakeLists.txt
  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • cmake/log_surgeon-config.cmake.in
📚 Learning: 2025-07-22T02:00:47.620Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 142
File: taskfiles/examples.yaml:63-76
Timestamp: 2025-07-22T02:00:47.620Z
Learning: CMake's find_package() command automatically honors <PackageName>_ROOT variables (e.g., LOG_SURGEON_ROOT) during its search process. The CMakeLists.txt file does not need to explicitly reference these _ROOT variables - they are built into CMake's package discovery mechanism.

Applied to files:

  • CMakeLists.txt
  • cmake/log_surgeon-config.cmake.in
📚 Learning: 2025-08-15T12:07:58.626Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 150
File: tests/test-expression-view.cpp:7-7
Timestamp: 2025-08-15T12:07:58.626Z
Learning: In tests/test-expression-view.cpp, the `<catch2/catch_message.hpp>` header is required for clang-tidy to pass, even though it may not be directly used in the visible code.

Applied to files:

  • CMakeLists.txt
  • tests/test-buffer-parser.cpp
  • README.md
📚 Learning: 2025-10-22T15:40:29.992Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:...

Applied to files:

  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • tests/test-buffer-parser.cpp
  • src/log_surgeon/Token.hpp
📚 Learning: 2025-12-03T16:38:15.354Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 199
File: src/log_surgeon/LogParser.cpp:154-155
Timestamp: 2025-12-03T16:38:15.354Z
Learning: In `src/log_surgeon/LogParser.cpp` and related parsing code, when `get_reg_ids_from_capture(capture)` returns an optional, calling `.value()` directly without checking is intentional for captures that should always have register IDs (e.g., `"timestamp"` captures). If the optional is nullopt, it indicates an internal programming error/invariant violation, and throwing `std::bad_optional_access` is the desired behavior rather than defensive error handling with continue/return.

Applied to files:

  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • tests/test-buffer-parser.cpp
  • src/log_surgeon/Token.hpp
📚 Learning: 2025-05-05T14:55:34.455Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lalr1Parser.tpp:661-665
Timestamp: 2025-05-05T14:55:34.455Z
Learning: The log-surgeon codebase follows a design approach where function contracts (like `ErrorCode::Success` guaranteeing a valid token) are trusted, and contract violations are allowed to throw exceptions rather than being explicitly checked at every call site.

Applied to files:

  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
  • tests/test-buffer-parser.cpp
📚 Learning: 2024-11-27T21:56:13.425Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T21:56:13.425Z
Learning: In the `log_surgeon` project, header guards in C++ header files should include `_HPP` at the end to match the filename. For example, in `RegisterHandler.hpp`, the header guard should be `LOG_SURGEON_FINITE_AUTOMATA_REGISTER_HANDLER_HPP`.

Applied to files:

  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-12-09T21:17:27.546Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 197
File: src/log_surgeon/finite_automata/RegexAST.hpp:1011-1019
Timestamp: 2025-12-09T21:17:27.546Z
Learning: In C++ files under src/log_surgeon in the y-scope/log-surgeon repository, for const pointer parameters in null checks, Yoda conditions (nullptr == ptr) are not required; standard comparison order (ptr == nullptr) is acceptable since const prevents accidental assignment. Apply this pattern to header and related source files within src/log_surgeon.

Applied to files:

  • src/log_surgeon/LogEvent.hpp
  • src/log_surgeon/Token.hpp
📚 Learning: 2025-09-11T11:16:39.441Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 162
File: src/log_surgeon/wildcard_query_parser/Query.cpp:112-0
Timestamp: 2025-09-11T11:16:39.441Z
Learning: In src/log_surgeon/wildcard_query_parser/Query.cpp, token payloads in get_all_single_token_interpretations() should use extended_view.get_search_string() rather than original_view.get_search_string(). The extended view (which includes adjacent greedy wildcards) has always been the intended content for token payloads, and this behavior matches the documented rules in the docstring.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2024-11-27T22:25:35.608Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T22:25:35.608Z
Learning: In the `RegisterHandler` class in `src/log_surgeon/finite_automata/RegisterHandler.hpp`, the methods `add_register` and `append_position` rely on `emplace_back` and `m_prefix_tree.insert` to handle exceptions correctly and maintain consistent state without requiring additional exception handling.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/Token.hpp
📚 Learning: 2024-11-13T20:02:13.737Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 48
File: src/log_surgeon/finite_automata/RegexNFAState.hpp:0-0
Timestamp: 2024-11-13T20:02:13.737Z
Learning: In `src/log_surgeon/finite_automata/RegexNFAState.hpp`, the constructor `RegexNFAState(std::set<Tag const*> tags, RegexNFAState const* dest_state)` has been updated to use `std::vector<Tag const*> tags` instead of `std::set`.

Applied to files:

  • tests/test-buffer-parser.cpp
  • src/log_surgeon/Token.hpp
📚 Learning: 2025-08-26T10:13:00.368Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 152
File: tests/test-query.cpp:61-67
Timestamp: 2025-08-26T10:13:00.368Z
Learning: In Catch2 unit tests, the REQUIRE macro already provides detailed debugging output when container comparisons (like std::set equality) fail, showing both expected and actual values. Additional CAPTURE statements are typically unnecessary for such comparisons.

Applied to files:

  • tests/test-buffer-parser.cpp
📚 Learning: 2025-08-08T13:30:25.172Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 144
File: tests/comparison_test_utils.hpp:0-0
Timestamp: 2025-08-08T13:30:25.172Z
Learning: In tests/comparison_test_utils.hpp (C++), all comparison helper templates (test_equal, test_greater_than, test_less_than, pairwise_comparison_of_strictly_ascending_vector) must be constrained with the StronglyThreeWayComparable concept on both declarations and definitions. Maintain this constraint in future changes.

Applied to files:

  • tests/test-buffer-parser.cpp
📚 Learning: 2025-11-06T11:16:52.917Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 184
File: tests/test-reader-parser.cpp:150-154
Timestamp: 2025-11-06T11:16:52.917Z
Learning: In Catch2 test files for the y-scope/log-surgeon repository, an explicit `if (false == optional.has_value()) { return; }` check may be required after `REQUIRE(optional.has_value())` to prevent clang-tidy errors, even though the code is unreachable at runtime. clang-tidy's static analyzer doesn't recognize that REQUIRE aborts execution and may flag unsafe optional access without the explicit check.

Applied to files:

  • tests/test-buffer-parser.cpp
📚 Learning: 2024-11-18T16:45:46.073Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 50
File: src/log_surgeon/finite_automata/Tag.hpp:0-0
Timestamp: 2024-11-18T16:45:46.073Z
Learning: The class `TagPositions` was removed from `src/log_surgeon/finite_automata/Tag.hpp` as it is no longer needed.

Applied to files:

  • src/log_surgeon/Token.hpp
📚 Learning: 2024-11-27T22:24:42.637Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/PrefixTree.hpp:80-80
Timestamp: 2024-11-27T22:24:42.637Z
Learning: The method `get_reversed_positions` is implemented in `src/log_surgeon/finite_automata/PrefixTree.cpp`.

Applied to files:

  • src/log_surgeon/Token.hpp
📚 Learning: 2025-12-19T07:58:22.871Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 201
File: src/log_surgeon/Token.cpp:38-58
Timestamp: 2025-12-19T07:58:22.871Z
Learning: In the log-surgeon codebase, the Token class intentionally supports wrapped buffers where start_pos > end_pos. Sub-tokens created via get_sub_token can legitimately wrap around the log buffer, and all Token class methods (get_cached_string, to_string_view, get_length, etc.) are designed to handle this scenario correctly. Do not suggest adding validation that enforces start_pos <= end_pos in get_sub_token.

Applied to files:

  • src/log_surgeon/Token.hpp
📚 Learning: 2025-08-08T10:23:06.281Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 144
File: src/log_surgeon/wildcard_query_parser/QueryInterpretation.hpp:80-88
Timestamp: 2025-08-08T10:23:06.281Z
Learning: In y-scope/log-surgeon (C++), small function definitions are allowed to remain inline in headers. For src/log_surgeon/wildcard_query_parser/QueryInterpretation.hpp, do not suggest moving small methods like QueryInterpretation::append_variable_token to the .cpp for “consistency” in future reviews.

Applied to files:

  • src/log_surgeon/Token.hpp
📚 Learning: 2024-12-04T11:20:27.990Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/PrefixTree.cpp:0-0
Timestamp: 2024-12-04T11:20:27.990Z
Learning: In the `PrefixTree` implementation in `src/log_surgeon/finite_automata/PrefixTree.cpp`, the root node serves as a common placeholder for all paths, and its position should be ignored in methods like `get_reversed_positions`.

Applied to files:

  • src/log_surgeon/Token.hpp
📚 Learning: 2025-08-08T10:00:20.963Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 144
File: src/log_surgeon/wildcard_query_parser/VariableQueryToken.hpp:28-31
Timestamp: 2025-08-08T10:00:20.963Z
Learning: In src/log_surgeon/wildcard_query_parser/QueryInterpretation.hpp (C++), do not default the comparison operators: the class stores std::vector<std::variant<StaticQueryToken, VariableQueryToken>>, which yields only a weak ordering. A custom operator<=> that maps the variant’s weak ordering to std::strong_ordering is required; avoid suggesting defaulting there in future reviews.

Applied to files:

  • src/log_surgeon/Token.hpp
🧬 Code graph analysis (4)
src/log_surgeon/LogEvent.hpp (2)
src/log_surgeon/Lexer.hpp (9)
  • nodiscard (154-154)
  • nodiscard (156-158)
  • nodiscard (160-162)
  • nodiscard (164-167)
  • nodiscard (176-182)
  • nodiscard (189-196)
  • nodiscard (204-211)
  • capture (189-191)
  • capture (219-220)
src/log_surgeon/finite_automata/Nfa.hpp (2)
  • capture (75-79)
  • capture (116-117)
src/log_surgeon/LogEvent.cpp (6)
src/log_surgeon/Lexer.hpp (5)
  • rule_id (66-67)
  • rule_id (74-75)
  • rule_id (176-177)
  • capture (189-191)
  • capture (219-220)
src/log_surgeon/BufferParser.hpp (1)
  • m_log_parser (76-76)
src/log_surgeon/ReaderParser.hpp (1)
  • m_log_parser (63-63)
src/log_surgeon/Token.hpp (2)
  • pos (122-122)
  • pos (129-129)
src/log_surgeon/LogEvent.hpp (2)
  • root_var (134-137)
  • root_var (153-154)
src/log_surgeon/finite_automata/Nfa.hpp (3)
  • captures (59-63)
  • capture (75-79)
  • capture (116-117)
tests/test-buffer-parser.cpp (2)
src/log_surgeon/Lexer.hpp (2)
  • capture (189-191)
  • capture (219-220)
tests/test-reader-parser.cpp (3)
  • parse_and_validate (66-70)
  • parse_and_validate (78-173)
  • parse_and_validate (78-82)
src/log_surgeon/Token.hpp (1)
src/log_surgeon/Lexer.hpp (2)
  • capture (189-191)
  • capture (219-220)
🪛 Cppcheck (2.19.0)
src/log_surgeon/LogEvent.cpp

[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 3-3: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 3-3: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[style] 73-73: The function 'get_mutable_active_buffer' is never used.

(unusedFunction)


[style] 76-76: The function 'add_token_group' is never used.

(unusedFunction)


[style] 84-84: The function 'add_token_chain' is never used.

(unusedFunction)


[style] 110-110: The function 'add_production' is never used.

(unusedFunction)


[style] 154-154: The function 'get_has_delimiters' is never used.

(unusedFunction)


[style] 156-156: The function 'get_delim_table' is never used.

(unusedFunction)


[style] 160-160: The function 'is_delimiter' is never used.

(unusedFunction)


[style] 86-86: The function 'set_input_buffer' is never used.

(unusedFunction)


[style] 94-94: The function 'get_input_pos' is never used.

(unusedFunction)


[style] 102-102: The function 'read_into_input' is never used.

(unusedFunction)


[style] 115-115: The function 'reset_log_event_view' is never used.

(unusedFunction)


[style] 120-120: The function 'get_log_event_view' is never used.

(unusedFunction)


[style] 68-68: The function 'add_delimiter' is never used.

(unusedFunction)


[style] 90-90: The function 'get_special_regex_characters' is never used.

(unusedFunction)


[style] 94-94: The function 'get_first_char' is never used.

(unusedFunction)


[style] 131-131: The function 'get_buffer_size' is never used.

(unusedFunction)


[style] 142-142: The function 'set_type_ids' is never used.

(unusedFunction)


[style] 125-125: The function 'get_tree_transitions' is never used.

(unusedFunction)


[style] 74-74: The function 'get_parent_id_unsafe' is never used.

(unusedFunction)


[style] 81-81: The function 'get_position' is never used.

(unusedFunction)


[style] 73-73: The function 'token_cast' is never used.

(unusedFunction)


[style] 81-81: The function 'non_terminal_cast' is never used.

(unusedFunction)


[style] 85-85: The function 'get_parser_ast' is never used.

(unusedFunction)


[style] 97-97: The function 'release_parser_ast' is never used.

(unusedFunction)


[style] 220-220: The function 'scan_with_wildcard' is never used.

(unusedFunction)

tests/test-buffer-parser.cpp

[information] 1-1: Include file

(missingIncludeSystem)


[performance] 50-50: Function parameter 'dfa_transition' should be passed by const reference.

(passedByValue)


[style] 41-41: The function 'increment_pos' is never used.

(unusedFunction)


[style] 47-47: The function 'double_size' is never used.

(unusedFunction)


[style] 53-53: The function 'static_size' is never used.

(unusedFunction)


[style] 39-39: The function 'set_timestamp' is never used.

(unusedFunction)


[style] 47-47: The function 'set_has_delimiters' is never used.

(unusedFunction)


[style] 49-49: The function 'has_delimiters' is never used.

(unusedFunction)


[style] 51-51: The function 'set_token' is never used.

(unusedFunction)


[style] 41-41: The function 'add_character' is never used.

(unusedFunction)


[style] 37-37: The function 'is_multi_valued' is never used.

(unusedFunction)


[style] 164-164: The function 'get_dfa' is never used.

(unusedFunction)


[performance] 439-439: Function 'get_ranges()' should return member 'm_ranges' by const reference.

(returnByReference)


[style] 422-422: The function 'add_range' is never used.

(unusedFunction)


[style] 424-424: The function 'add_literal' is never used.

(unusedFunction)


[style] 426-426: The function 'set_is_wildcard_true' is never used.

(unusedFunction)


[style] 428-428: The function 'negate' is never used.

(unusedFunction)


[style] 435-435: The function 'is_wildcard' is never used.

(unusedFunction)


[style] 437-437: The function 'is_negated' is never used.

(unusedFunction)


[style] 439-439: The function 'get_ranges' is never used.

(unusedFunction)

⏰ 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: build (ubuntu-24.04, release)
  • GitHub Check: build (macos-15, release)
  • GitHub Check: build (ubuntu-24.04, debug)
  • GitHub Check: build (macos-15, debug)
  • GitHub Check: build (ubuntu-22.04, debug)
  • GitHub Check: build (ubuntu-22.04, release)
  • GitHub Check: lint (ubuntu-24.04)
  • GitHub Check: lint (macos-15)
🔇 Additional comments (17)
src/.clang-format (1)

12-12: LGTM!

The regex pattern correctly includes ystdlib in the external library headers category, ensuring consistent header grouping with other external dependencies.

README.md (2)

58-58: LGTM!

Good improvement to capture the LogEventView reference once at the start of the loop body, avoiding repeated calls to get_log_event_view().


94-94: LGTM!

The new ystdlib-cpp dependency is properly documented with version requirement and reference link.

Also applies to: 197-197

taskfiles/deps.yaml (1)

35-46: LGTM!

The Boost installation task is properly configured with SHA256 verification and targets only the headers component needed by ystdlib.

cmake/log_surgeon-config.cmake.in (1)

13-15: LGTM!

The conditional dependency follows the established pattern for other dependencies and is correctly placed before the include directory setup.

CMakeLists.txt (2)

55-56: LGTM!

The ystdlib dependency is properly integrated with version requirement and status message, consistent with other dependencies. Based on learnings, using Result<T, ErrorCode> from ystdlib-cpp is the preferred error handling approach in log-surgeon.


143-143: LGTM!

PUBLIC linkage for ystdlib::error_handling is appropriate since the error handling types are exposed in log_surgeon's public API.

tests/test-buffer-parser.cpp (3)

37-49: LGTM! Well-designed test helper struct.

The ExpectedCaptureMatch struct cleanly encapsulates the expected capture data including the new m_leaf flag. The constructor overload defaulting m_leaf to true is a sensible convenience for most test cases where captures are leaves.


163-207: Thorough API validation approach.

The test now validates both the direct field access (lines 145-162) and the new event API (lines 164-207). The sorting logic correctly orders captures by start position (ascending) then end position (descending) to match the API's documented behaviour. The leaf detection logic at lines 194-199 correctly handles captures with no match.


448-474: Good coverage for nested capture groups.

This new test case validates the key functionality of this PR—correctly identifying leaf vs. non-leaf captures in nested structures. The expected logtype "userID=abc_<uid> userID=abc_<uid>" confirms only leaf captures are substituted, while the parent full capture is marked with false for m_leaf.

src/log_surgeon/Token.hpp (2)

20-47: Clean API design for capture match representation.

The new types are well-structured:

  • position_t type alias reduces verbosity
  • CaptureMatchPosition is a simple value type for position pairs
  • CaptureMatch bundles capture metadata with position and leaf status

The documentation clearly explains the semantics of m_leaf.


107-110: Consistent use of the new type alias.

The return type update from std::vector<finite_automata::PrefixTree::position_t> to std::vector<position_t> improves readability.

src/log_surgeon/LogEvent.hpp (1)

185-193: Error handling infrastructure follows project conventions.

The error code enum and category setup correctly follows the ystdlib error handling pattern as established in the project learnings. The macro YSTDLIB_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM is placed outside the namespace as required.

src/log_surgeon/LogEvent.cpp (4)

58-98: Clean refactoring of get_logtype to use the new API.

The refactored implementation correctly:

  1. Handles uncaught strings directly
  2. Manages delimiters for non-first, non-newline tokens
  3. Falls back to the rule symbol when no captures exist
  4. Only substitutes leaf capture matches into the logtype

The logic at lines 88-94 properly iterates matches and inserts static text between capture boundaries.


145-160: Correct implementation of get_capture_position.

The function correctly:

  1. Retrieves register IDs from the capture
  2. Gets reversed positions from both start and end registers
  3. Validates that positions exist and are non-negative before returning
  4. Returns appropriate error codes for missing matches

The Yoda condition 0 > start_positions[0] at line 154 correctly follows the project's style for comparisons.


207-227: Error category implementation is complete.

The template specializations for name() and message() correctly provide descriptive strings for debugging and error reporting. The default case in the switch handles potential future error codes gracefully.


100-143: Consider using coding guideline pattern for empty check.

The logic is sound—using a std::set with a custom comparator ensures captures are sorted correctly, and the leaf detection at lines 136-138 properly checks if a capture's end is before the next capture's start.

However, line 126 uses ordered_matches.empty() which per coding guidelines should prefer false == <expression> over !<expression>. While empty() is a method call rather than !, the pattern could be true == ordered_matches.empty() for consistency.

♻️ Optional: Apply coding guideline pattern
-    if (ordered_matches.empty()) {
+    if (true == ordered_matches.empty()) {
         return {{}};
     }
⛔ Skipped due to learnings
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 152
File: tests/test-query.cpp:61-67
Timestamp: 2025-08-26T10:13:00.368Z
Learning: In Catch2 unit tests, the REQUIRE macro already provides detailed debugging output when container comparisons (like std::set equality) fail, showing both expected and actual values. Additional CAPTURE statements are typically unnecessary for such comparisons.
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 199
File: src/log_surgeon/LogParser.cpp:154-155
Timestamp: 2025-12-03T16:38:15.354Z
Learning: In `src/log_surgeon/LogParser.cpp` and related parsing code, when `get_reg_ids_from_capture(capture)` returns an optional, calling `.value()` directly without checking is intentional for captures that should always have register IDs (e.g., `"timestamp"` captures). If the optional is nullopt, it indicates an internal programming error/invariant violation, and throwing `std::bad_optional_access` is the desired behavior rather than defensive error handling with continue/return.
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 144
File: src/log_surgeon/wildcard_query_parser/VariableQueryToken.hpp:28-31
Timestamp: 2025-08-08T10:00:20.963Z
Learning: In src/log_surgeon/wildcard_query_parser/QueryInterpretation.hpp (C++), do not default the comparison operators: the class stores std::vector<std::variant<StaticQueryToken, VariableQueryToken>>, which yields only a weak ordering. A custom operator<=> that maps the variant’s weak ordering to std::strong_ordering is required; avoid suggesting defaulting there in future reviews.
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 184
File: tests/test-reader-parser.cpp:150-154
Timestamp: 2025-11-06T11:16:52.917Z
Learning: In Catch2 test files for the y-scope/log-surgeon repository, an explicit `if (false == optional.has_value()) { return; }` check may be required after `REQUIRE(optional.has_value())` to prevent clang-tidy errors, even though the code is unreachable at runtime. clang-tidy's static analyzer doesn't recognize that REQUIRE aborts execution and may flag unsafe optional access without the explicit check.
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:` rules only.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @README.md:
- Line 58: The code declares a reference "LogEventView const& event =
parser.get_log_parser().get_log_event_view()" but later calls
"event->to_string()"; change that arrow usage to the dot operator (call
"event.to_string()") wherever "event->to_string()" appears so you're invoking
the method on the reference correctly (locate the to_string call tied to
LogEventView const& event and replace the arrow with a dot).

In @src/log_surgeon/LogEvent.cpp:
- Around line 217-226: The message for LogEventErrorCodeEnum::NoCaptureGroups in
LogEventErrorCategory::message is inconsistent (singular "NoCaptureGroup");
update the returned string in the LogEventErrorCategory::message specialization
to match the enum name (e.g., "NoCaptureGroups" or a clearer phrasing like "No
capture groups") so the enum LogEventErrorCodeEnum::NoCaptureGroups and its
message are consistent for debugging.
- Around line 119-124: The current error check only skips when position.error()
equals LogEventErrorCode::NoCaptureGroupMatch before calling position.value()
for emplacing into ordered_matches, which would break if new error codes are
introduced; change the logic around position (the result from
get_capture_position) to simply skip on any error (i.e., if position.has_error()
then continue) before calling position.value(), ensuring
ordered_matches.emplace(capture, position.value(), true) only runs on successful
results.
♻️ Duplicate comments (1)
src/log_surgeon/LogEvent.hpp (1)

128-141: API declaration looks correct; past docstring issue has been addressed.

The docstring now correctly documents LogEventErrorCodeEnum::NoCaptureGroupMatch as the error code, resolving the previous review concern.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7375f2 and d246407.

📒 Files selected for processing (3)
  • README.md
  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

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

Files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
🧠 Learnings (13)
📓 Common learnings
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:...
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lalr1Parser.tpp:661-665
Timestamp: 2025-05-05T14:55:34.455Z
Learning: The log-surgeon codebase follows a design approach where function contracts (like `ErrorCode::Success` guaranteeing a valid token) are trusted, and contract violations are allowed to throw exceptions rather than being explicitly checked at every call site.
📚 Learning: 2025-12-03T16:22:52.313Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 199
File: src/log_surgeon/LogParser.cpp:195-195
Timestamp: 2025-12-03T16:22:52.313Z
Learning: In y-scope/log-surgeon, when reviewing code, do not suggest fixing style issues (such as C-style casts vs. static_cast) in lines that were not modified by the PR. The maintainer prefers to keep PRs focused on their intended changes and defer broader style refactoring to dedicated refactor PRs.

Applied to files:

  • README.md
📚 Learning: 2025-10-22T15:40:29.992Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 165
File: src/log_surgeon/LogEvent.cpp:53-55
Timestamp: 2025-10-22T15:40:29.992Z
Learning: In `src/log_surgeon/LogEvent.cpp`, the `get_logtype()` method has two independent mechanisms for adding `<timestamp>` to the logtype string: (1) A prefix added when `has_timestamp()` returns true, indicating a standalone timestamp token from timestamp rules (lines 53-55), and (2) Named capture group processing that adds `<timestamp>` tags for `(?<timestamp>...)` patterns in any variable rule (lines 61-95). These mechanisms do not interfere with each other. Capture groups named `timestamp` do not affect `has_timestamp()` status, as using captures as actual timestamps is not supported.
<!-- [add_learning]
In the log-surgeon codebase, capture groups (e.g., `(?<timestamp>...)`) in schema variable rules are added to the logtype string through the existing capture group processing logic, but they do not create actual timestamp tokens and do not cause `has_timestamp()` to return true. Using capture groups as actual timestamps is not supported; standalone timestamp tokens come from `timestamp:...

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2025-09-11T11:16:39.441Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 162
File: src/log_surgeon/wildcard_query_parser/Query.cpp:112-0
Timestamp: 2025-09-11T11:16:39.441Z
Learning: In src/log_surgeon/wildcard_query_parser/Query.cpp, token payloads in get_all_single_token_interpretations() should use extended_view.get_search_string() rather than original_view.get_search_string(). The extended view (which includes adjacent greedy wildcards) has always been the intended content for token payloads, and this behavior matches the documented rules in the docstring.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-12-03T16:38:15.354Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 199
File: src/log_surgeon/LogParser.cpp:154-155
Timestamp: 2025-12-03T16:38:15.354Z
Learning: In `src/log_surgeon/LogParser.cpp` and related parsing code, when `get_reg_ids_from_capture(capture)` returns an optional, calling `.value()` directly without checking is intentional for captures that should always have register IDs (e.g., `"timestamp"` captures). If the optional is nullopt, it indicates an internal programming error/invariant violation, and throwing `std::bad_optional_access` is the desired behavior rather than defensive error handling with continue/return.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2025-08-08T10:23:06.281Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 144
File: src/log_surgeon/wildcard_query_parser/QueryInterpretation.hpp:80-88
Timestamp: 2025-08-08T10:23:06.281Z
Learning: In y-scope/log-surgeon (C++), small function definitions are allowed to remain inline in headers. For src/log_surgeon/wildcard_query_parser/QueryInterpretation.hpp, do not suggest moving small methods like QueryInterpretation::append_variable_token to the .cpp for “consistency” in future reviews.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-05-01T14:47:57.016Z
Learnt from: davidlion
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2024-11-27T21:56:13.425Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T21:56:13.425Z
Learning: In the `log_surgeon` project, header guards in C++ header files should include `_HPP` at the end to match the filename. For example, in `RegisterHandler.hpp`, the header guard should be `LOG_SURGEON_FINITE_AUTOMATA_REGISTER_HANDLER_HPP`.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2024-11-27T22:25:35.608Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T22:25:35.608Z
Learning: In the `RegisterHandler` class in `src/log_surgeon/finite_automata/RegisterHandler.hpp`, the methods `add_register` and `append_position` rely on `emplace_back` and `m_prefix_tree.insert` to handle exceptions correctly and maintain consistent state without requiring additional exception handling.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
📚 Learning: 2025-08-26T10:06:22.914Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 152
File: src/log_surgeon/wildcard_query_parser/Query.hpp:9-11
Timestamp: 2025-08-26T10:06:22.914Z
Learning: In y-scope/log-surgeon project, it's acceptable to include headers like log_surgeon/Lexer.hpp directly in header files rather than using forward declarations, even when only references are used in the interface. The project prefers the simplicity of direct includes over header coupling optimization through forward declarations.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2025-08-15T01:04:03.681Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 150
File: src/log_surgeon/wildcard_query_parser/ExpressionView.cpp:11-11
Timestamp: 2025-08-15T01:04:03.681Z
Learning: In y-scope/log-surgeon C++ codebase, prefer explicit includes in implementation files (.cpp) for types that are directly used, even when those headers are already transitively included through the corresponding header file. This makes dependencies explicit and improves maintainability.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2025-05-05T14:55:34.455Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 106
File: src/log_surgeon/Lalr1Parser.tpp:661-665
Timestamp: 2025-05-05T14:55:34.455Z
Learning: The log-surgeon codebase follows a design approach where function contracts (like `ErrorCode::Success` guaranteeing a valid token) are trusted, and contract violations are allowed to throw exceptions rather than being explicitly checked at every call site.

Applied to files:

  • src/log_surgeon/LogEvent.cpp
  • src/log_surgeon/LogEvent.hpp
📚 Learning: 2025-12-09T21:17:27.546Z
Learnt from: SharafMohamed
Repo: y-scope/log-surgeon PR: 197
File: src/log_surgeon/finite_automata/RegexAST.hpp:1011-1019
Timestamp: 2025-12-09T21:17:27.546Z
Learning: In C++ files under src/log_surgeon in the y-scope/log-surgeon repository, for const pointer parameters in null checks, Yoda conditions (nullptr == ptr) are not required; standard comparison order (ptr == nullptr) is acceptable since const prevents accidental assignment. Apply this pattern to header and related source files within src/log_surgeon.

Applied to files:

  • src/log_surgeon/LogEvent.hpp
🧬 Code graph analysis (2)
src/log_surgeon/LogEvent.cpp (2)
src/log_surgeon/Token.hpp (2)
  • pos (122-122)
  • pos (129-129)
src/log_surgeon/LogEvent.hpp (2)
  • root_var (138-141)
  • root_var (156-157)
src/log_surgeon/LogEvent.hpp (2)
src/log_surgeon/LogEvent.cpp (2)
  • nodiscard (41-43)
  • nodiscard (45-56)
src/log_surgeon/finite_automata/Nfa.hpp (2)
  • capture (75-79)
  • capture (116-117)
🪛 Cppcheck (2.19.0)
src/log_surgeon/LogEvent.cpp

[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 3-3: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 3-3: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 15-15: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 16-16: Include file

(missingIncludeSystem)


[information] 17-17: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 8-8: Include file

(missingIncludeSystem)


[information] 9-9: Include file

(missingIncludeSystem)


[information] 10-10: Include file

(missingIncludeSystem)


[information] 11-11: Include file

(missingIncludeSystem)


[information] 12-12: Include file

(missingIncludeSystem)


[information] 13-13: Include file

(missingIncludeSystem)


[information] 14-14: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[information] 7-7: Include file

(missingIncludeSystem)


[information] 4-4: Include file

(missingIncludeSystem)


[information] 5-5: Include file

(missingIncludeSystem)


[information] 6-6: Include file

(missingIncludeSystem)


[style] 73-73: The function 'get_mutable_active_buffer' is never used.

(unusedFunction)


[style] 76-76: The function 'add_token_group' is never used.

(unusedFunction)


[style] 84-84: The function 'add_token_chain' is never used.

(unusedFunction)


[style] 110-110: The function 'add_production' is never used.

(unusedFunction)


[style] 154-154: The function 'get_has_delimiters' is never used.

(unusedFunction)


[style] 156-156: The function 'get_delim_table' is never used.

(unusedFunction)


[style] 160-160: The function 'is_delimiter' is never used.

(unusedFunction)


[style] 86-86: The function 'set_input_buffer' is never used.

(unusedFunction)


[style] 94-94: The function 'get_input_pos' is never used.

(unusedFunction)


[style] 102-102: The function 'read_into_input' is never used.

(unusedFunction)


[style] 115-115: The function 'reset_log_event_view' is never used.

(unusedFunction)


[style] 120-120: The function 'get_log_event_view' is never used.

(unusedFunction)


[style] 68-68: The function 'add_delimiter' is never used.

(unusedFunction)


[style] 90-90: The function 'get_special_regex_characters' is never used.

(unusedFunction)


[style] 94-94: The function 'get_first_char' is never used.

(unusedFunction)


[style] 131-131: The function 'get_buffer_size' is never used.

(unusedFunction)


[style] 142-142: The function 'set_type_ids' is never used.

(unusedFunction)


[style] 125-125: The function 'get_tree_transitions' is never used.

(unusedFunction)


[style] 74-74: The function 'get_parent_id_unsafe' is never used.

(unusedFunction)


[style] 81-81: The function 'get_position' is never used.

(unusedFunction)


[style] 73-73: The function 'token_cast' is never used.

(unusedFunction)


[style] 81-81: The function 'non_terminal_cast' is never used.

(unusedFunction)


[style] 85-85: The function 'get_parser_ast' is never used.

(unusedFunction)


[style] 97-97: The function 'release_parser_ast' is never used.

(unusedFunction)


[style] 220-220: The function 'scan_with_wildcard' is never used.

(unusedFunction)

⏰ 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: build (ubuntu-24.04, release)
  • GitHub Check: build (macos-15, release)
  • GitHub Check: build (ubuntu-22.04, release)
  • GitHub Check: build (macos-15, debug)
  • GitHub Check: build (ubuntu-24.04, debug)
  • GitHub Check: build (ubuntu-22.04, debug)
  • GitHub Check: lint (macos-15)
  • GitHub Check: lint (ubuntu-24.04)
🔇 Additional comments (8)
README.md (2)

94-94: LGTM: ystdlib-cpp dependency added correctly.

The new dependency on ystdlib-cpp (>= 0.1.0) is properly documented in both the requirements list and the reference links section, aligning with the PR's integration of ystdlib error-handling.

Also applies to: 197-197


58-58: LGTM: Event binding improves code clarity.

Introducing the local event binding streamlines the example by avoiding repeated calls to get_log_parser().get_log_event_view() and aligns well with the new capture helper APIs introduced in this PR.

src/log_surgeon/LogEvent.hpp (3)

4-16: LGTM! Includes are properly organized.

The new includes for ystdlib error handling infrastructure and Capture.hpp are correctly added to support the new API. Based on learnings, using Result<T, ErrorCode> from ystdlib-cpp is the preferred error handling pattern for this codebase.


143-157: Well-documented API for retrieving sorted capture matches.

The docstring clearly explains the sorting behaviour and leaf detection logic. The return type using ystdlib's Result type follows the established error handling patterns from learnings.


188-196: Error code infrastructure follows ystdlib patterns.

The error enum, type alias, and macro usage align with the established error handling patterns in the codebase.

src/log_surgeon/LogEvent.cpp (3)

58-98: Refactored get_logtype() correctly uses the new capture API.

The implementation properly handles the error case by falling back to the rule symbol name, and only substitutes leaf capture group matches as intended by the PR objectives.


100-143: get_capture_matches implementation correctly determines leaf captures.

The sorting logic (by increasing start position, then decreasing end position) and leaf detection (comparing end positions) correctly implement the documented behaviour for nested capture groups.


145-160: get_capture_position implementation is correct.

The function properly validates that both start and end positions exist and are non-negative before returning them.

logtype.append("<" + m_log_parser.get_id_symbol(rule_id) + ">");
continue;
}
auto pos{token_view.get_start_pos()};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better name than pos to make the logic clear. Something like prev_end_pos.

Comment on lines 116 to 117
for (size_t i{0}; i < captures->size(); ++i) {
auto const* const capture{captures->at(i)};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to do:
for (auto const* const capture : captures)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sorry, that is from old code, but it should be updated now like you say.

std::vector<Token::CaptureMatch> matches;
matches.reserve(ordered_matches.size());
auto const last_match{std::prev(ordered_matches.end())};
for (auto match = ordered_matches.begin(); match != last_match; ++match) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto match = ordered_matches.begin(); match != last_match; ++match) {
for (auto match{ordered_matches.begin()}; match != last_match; ++match) {

}

auto LogEventView::get_capture_position(
Token const& root_variable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its named root_var in the above method, so maybe use the same name here

if (LogEventErrorCode{LogEventErrorCodeEnum::NoCaptureGroupMatch} == position.error()) {
continue;
}
return position.error();
Copy link
Contributor

@SharafMohamed SharafMohamed Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is never currently reached right? Even if it was reachable, It seems like for it to be reached something has to be broken, but currently the code will just continue treating it as a non-capture var.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya currently get_capture_position can only return NoCaptureGroupMatch, but this error handling is more future proof since any other error should cause get_capture_matches to fail.

Inside log surgeon get_capture_matches is only used in get_logtype which doesn't handle any errors, but the clp code that uses get_capture_matches would error. We could make get_logtype return a Result if we don't want it to always return something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main question is what should happen if get_capture_matches fails with any other type of error. In the case of NoCaptureGroupMatch its clear that this is just a non-capture group, so get_logtype will just treat it as a non-capture and continue. But, with other error types it'll do the same which is probably wrong.

That being said, as of now the behavior is correct, so this could just be left as is and addressed in the future if a case does come up.

Copy link
Contributor

@SharafMohamed SharafMohamed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments and changes.

Copy link
Contributor

@SharafMohamed SharafMohamed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@SharafMohamed SharafMohamed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@davidlion davidlion merged commit 193e1f9 into y-scope:main Jan 16, 2026
10 of 11 checks passed
@davidlion davidlion deleted the capture-helpers branch January 16, 2026 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants