refactor(clp-s): Add ANTLR-generated parsers to codebase to eliminate need for code dependencies at build time.#925
Conversation
## Walkthrough
This change migrates ANTLR parser generation from CMake macros to an explicit task-based system using Taskfile, removes custom CMake modules for ANTLR integration, and relies on pre-generated parser sources. Build and lint configurations are updated to accommodate this, and a new GitHub Actions workflow ensures generated code is properly committed.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| components/core/CMakeLists.txt | Switched from custom ANTLR detection to using `antlr4-runtime` via CMake; removed explicit fatal error and comments about static library building. |
| components/core/cmake/Modules/FindANTLR.cmake<br>components/core/cmake/Modules/ExternalAntlr4Cpp.cmake | Deleted custom CMake modules for ANTLR detection, parser generation, and external ANTLR4 C++ runtime management. |
| components/core/src/clp_s/search/kql/CMakeLists.txt<br>components/core/src/clp_s/search/sql/CMakeLists.txt | Removed `ANTLR_TARGET` macro usage; now explicitly lists generated parser files and includes them directly in the build. |
| components/core/src/clp_s/search/kql/kql.cpp<br>components/core/src/clp_s/search/sql/sql.cpp | Changed `using` directives to import parser classes from the `generated` namespace instead of the previous one. |
| taskfile.yaml | Added `clp-s-generate-parsers` task for ANTLR parser generation; updated `core-generate` to depend on this task. |
| taskfiles/lint.yaml | Updated lint and formatting tasks to exclude generated parser files from checks. |
| taskfiles/deps/main.yaml | Split ANTLR dependency into `antlr-jar` (downloads JAR) and `antlr-runtime` (builds C++ runtime); updated dependency graph accordingly. |
| .github/workflows/clp-s-generated-code-checks.yaml | Added workflow to ensure generated parser code is committed and not out-of-date on PRs and pushes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Developer
participant Taskfile
participant ANTLR Tool
participant Generated Sources
participant CMake Build
Developer->>Taskfile: Run clp-s-generate-parsers
Taskfile->>ANTLR Tool: Invoke with Kql.g4 and Sql.g4
ANTLR Tool->>Generated Sources: Output C++ parser files
Taskfile->>Generated Sources: Remove non-C++ files, checksum outputs
Developer->>CMake Build: Build project
CMake Build->>Generated Sources: Include explicit parser files in buildSuggested reviewers
Learnt from: gibber9809 Learnt from: anlowee |
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (1)
1-902: 🧹 Nitpick (assertive)Generated code contains coding guideline violations.
This ANTLR-generated implementation file is properly structured but contains violations of the project's coding guidelines that prefer
false == <expression>over!<expression>.Specific violations found:
- Line 402:
if (!(precpred(_ctx, 2)))- Lines 842-843:
if (!((((_la & ~ 0x3fULL) == 0) && ((1ULL << _la) & 448) != 0)))Since this is generated code (as indicated by line 2), these issues should ideally be addressed by:
- Modifying the ANTLR code generation templates if possible
- Adding a post-processing step to fix these patterns
- Excluding generated files from this specific linting rule
The generated code is otherwise correct and functional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (32)
components/core/CMakeLists.txt(1 hunks)components/core/cmake/Modules/FindANTLR.cmake(0 hunks)components/core/src/clp_s/search/kql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/Kql.interp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/Kql.tokens(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.interp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.tokens(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/antlr4-runtime.h(1 hunks)components/core/src/clp_s/search/sql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/Sql.interp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/Sql.tokens(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.interp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.tokens(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/antlr4-runtime.h(1 hunks)taskfile.yaml(1 hunks)taskfiles/lint.yaml(4 hunks)
💤 Files with no reviewable changes (1)
- components/core/cmake/Modules/FindANTLR.cmake
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlParser.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp
🧬 Code Graph Analysis (5)
components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h (1)
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (16)
ctx(20-22)ctx(20-20)ctx(24-26)ctx(24-24)ctx(28-30)ctx(28-28)ctx(32-34)ctx(32-32)ctx(36-38)ctx(36-36)ctx(40-42)ctx(40-40)ctx(44-46)ctx(44-44)ctx(48-50)ctx(48-48)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (1)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (2)
SqlLexer(95-98)SqlLexer(100-102)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (1)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (2)
KqlLexer(154-157)KqlLexer(159-161)
components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h (1)
components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp (3)
SqlParser(83-83)SqlParser(85-88)SqlParser(90-92)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (3)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h (35)
i(130-130)i(227-227)KqlParser(27-27)KqlParser(29-29)KqlParser(31-31)StartContext(55-55)visitor(61-61)visitor(86-86)visitor(98-98)visitor(109-109)visitor(119-119)visitor(134-134)visitor(148-148)visitor(167-167)visitor(187-187)visitor(200-200)visitor(213-213)visitor(233-233)_localctx(240-240)_localctx(242-242)QueryContext(69-69)QueryContext(71-71)ExprContext(82-82)NestedQueryContext(91-91)NotQueryContext(103-103)SubQueryContext(114-114)OrAndQueryContext(124-124)precedence(138-138)ExpressionContext(141-141)Column_range_expressionContext(159-159)Column_value_expressionContext(179-179)ColumnContext(195-195)Value_expressionContext(208-208)List_of_valuesContext(224-224)context(72-72)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (16)
ctx(20-22)ctx(20-20)ctx(24-26)ctx(24-24)ctx(28-30)ctx(28-28)ctx(32-34)ctx(32-32)ctx(36-38)ctx(36-36)ctx(40-42)ctx(40-40)ctx(44-46)ctx(44-44)ctx(48-50)ctx(48-48)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h (12)
context(23-23)context(25-25)context(27-27)context(29-29)context(31-31)context(33-33)context(35-35)context(37-37)context(39-39)context(41-41)context(43-43)context(45-45)
🪛 GitHub Check: lint-check (ubuntu-latest)
taskfile.yaml
[failure] 97-97:
97:101 [line-length] line too long (220 > 100 characters)
[failure] 96-96:
96:101 [line-length] line too long (220 > 100 characters)
[failure] 95-95:
95:101 [line-length] line too long (105 > 100 characters)
taskfiles/lint.yaml
[failure] 138-138:
138:28 [trailing-spaces] trailing spaces
[failure] 152-152:
152:28 [trailing-spaces] trailing spaces
🪛 GitHub Actions: clp-lint
taskfile.yaml
[error] 95-101: line-length error: line too long (105 > 100 characters)
🪛 YAMLlint (1.37.1)
taskfiles/lint.yaml
[error] 138-138: trailing spaces
(trailing-spaces)
[error] 152-152: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
🔇 Additional comments (30)
components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cpp (1)
1-10: Skip auto-generated visitor implementation
This file is produced by ANTLR 4.13.1 and contains no handwritten logic. It’s already excluded from linting and formatting according to the updated configuration, so no manual review is required.components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.tokens (1)
1-2: Skip auto-generated token definition
This token file is generated by ANTLR and part of the SQL lexer output. It’s excluded from static analysis; no manual review needed.components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cpp (1)
1-10: Skip auto-generated base visitor implementation
As with other ANTLR outputs, this file is machine-generated boilerplate and is excluded from lint/format rules. No changes are required.components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cpp (1)
1-10: Skip auto-generated visitor implementation
This file is part of the ANTLR-generated SQL visitor stubs. It’s excluded from manual review and static analysis as per the updated lint configuration.components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cpp (1)
1-10: Skip auto-generated base visitor implementation
Generated by ANTLR 4.13.1, this file provides default visitor methods and is excluded from formatting and analysis checks.components/core/src/clp_s/search/sql/antlr_generated/Sql.tokens (1)
1-1: Skip review for generated file.
This file is auto-generated by ANTLR; manual modifications are not recommended.components/core/CMakeLists.txt (1)
121-124: Ensure theExternalAntlr4Cppmodule is available.
The newinclude(ExternalAntlr4Cpp)replaces the previous ANTLR lookup. Verify thatExternalAntlr4Cpp.cmakeexists in thecmake/Modules/directory (or another directory onCMAKE_MODULE_PATH) and correctly defines the ANTLR C++ runtime and associated targets.components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.interp (1)
1-19: Skip review for generated file.
This file is produced by ANTLR; manual edits will be overwritten on regeneration.components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.h (1)
1-29: Skip review for generated file.
This header is auto-generated by ANTLR; avoid manual edits.components/core/src/clp_s/search/sql/antlr_generated/Sql.interp (1)
1-14: ANTLR-Generated Interpreter Definition Looks Good
This.interpfile is purely a static ATN data resource produced by ANTLR. There’s no executable logic to validate beyond auto-generation consistency.components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h (1)
17-23: Base Visitor Method Correctly Delegates to Children
The override ofvisitStartmatches the pattern used in the KQL equivalent and delegates properly viavisitChildren. No changes needed.components/core/src/clp_s/search/kql/CMakeLists.txt (2)
8-18: Explicit Generated-Code Listing Is Correct
Theset(ANTLR_GENERATED_KQL_CODE ...)block accurately enumerates all generated.cppand.hfiles, fulfilling the PR’s goal of embedding ANTLR outputs directly.
29-29: Include Directory Configuration Valid
target_include_directories(kql PRIVATE ${ANTLR_GENERATED_KQL_CODE_DIR})ensures the compiler can locate the generated headers. This aligns with standard practice for generated sources.components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.tokens (1)
1-20: Auto-Generated Token Map Is Accurate
This.tokensfile is produced by ANTLR and correctly maps each KQL token to its numeric type. No manual edits are required.components/core/src/clp_s/search/kql/antlr_generated/Kql.tokens (1)
1-19: Skip manual review: generated code
This is an auto-generated ANTLR tokens file for KQL. No manual review needed.components/core/src/clp_s/search/sql/CMakeLists.txt (3)
1-7: Removed ANTLR_TARGET macro correctly
Disabling the oldANTLR_TARGETinvocation aligns with dropping the custom CMake module. Ensure that the externalExternalAntlr4Cppis included upstream incomponents/core/CMakeLists.txtto provide the ANTLR4 runtime & compiler.
21-27: Verify generated source variable usage
Theadd_libraryinvocation correctly includes${ANTLR_GENERATED_SQL_CODE}. Double-check that the variable expansion matches the expected list format and doesn’t introduce empty entries if the directory is missing or empty in some build scenarios.
29-29: Validate include directory visibility
UsingPRIVATEfortarget_include_directoriesis appropriate if no downstream target needs to see these headers. Confirm that none of your consumers ofsqlrequire those ANTLR headers directly; otherwise considerPUBLIC.components/core/src/clp_s/search/sql/antlr_generated/antlr4-runtime.h (1)
1-169: Skip manual review: ANTLR4 runtime umbrella header
This is auto-generated ANTLR4 C++ runtime code. No manual changes should be made here; ensure that lint and formatting tools skip this directory.components/core/src/clp_s/search/kql/antlr_generated/Kql.interp (1)
1-200: Skip manual review: generated interpreter metadata
Auto-generated ANTLR parser interpreter data for KQL. Ensure this directory is excluded from lint and static analysis.components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.interp (1)
1-300: Skip generated code
This file is entirely auto-generated by ANTLR and should not be manually reviewed or modified.components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (1)
1-60: Skip generated code
This header is auto-generated by ANTLR from the grammar and should not be manually altered.components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (1)
1-150: Skip generated code
This implementation is auto-generated by ANTLR and should remain untouched.components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (1)
1-60: Skip generated code
This header is generated by ANTLR and is not intended for manual edits.components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h (1)
1-50: Skip generated code
This visitor interface is auto-generated by ANTLR; manual reviews aren’t necessary.components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (1)
1-72: LGTM! Standard ANTLR-generated visitor base class.The generated KqlBaseVisitor class follows the expected ANTLR visitor pattern with proper default implementations.
components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h (1)
1-65: LGTM! Minimal ANTLR-generated SQL parser header.The generated SqlParser class follows standard ANTLR patterns. The minimal implementation with only a SPACE token and start rule suggests this might be a placeholder for future SQL parsing functionality.
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (1)
1-201:Details
✅ Verification successful
Verify ANTLR version consistency across generated files.
The generated code is from ANTLR 4.13.1. Ensure all ANTLR-generated files in the project use the same version to avoid potential compatibility issues.
🏁 Script executed:
#!/bin/bash # Description: Check ANTLR version consistency across all generated files # Search for ANTLR version declarations in generated files fd -e cpp -e h -e java . --exec grep -l "ANTLR 4\." {} \; | xargs grep -h "Generated from.*by ANTLR" | sort | uniq -cLength of output: 312
ANTLR Version Consistency Verified
All ANTLR-generated files in the project (Kql and Sql grammars) are using version 4.13.1, so there’s no action required.
components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp (1)
1-171: LGTM! Standard ANTLR-generated SQL parser implementation.The generated SqlParser implementation correctly matches its header file and follows expected ANTLR patterns.
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h (1)
1-253: LGTM! Generated parser header is properly structured.This ANTLR-generated header file correctly defines the parser interface, token types, and parse tree context classes. Since this is generated code (as indicated by line 2), any future modifications should be made to the source grammar file
Kql.g4rather than editing this file directly.
gibber9809
left a comment
There was a problem hiding this comment.
Nice work!
Just have a suggestion about the namespace for the generated code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
taskfile.yaml (1)
94-102: 🧹 Nitpick (assertive)Encapsulate ANTLR version and JAR path into variables
Hardcoding the version (4.13.1), jar name, and/tmppath inflates line length (triggering the lint error) and complicates future upgrades. Define at the top undervars:vars: ANTLR_VERSION: "4.13.1" ANTLR_JAR_NAME: "antlr-{{.ANTLR_VERSION}}-complete.jar" ANTLR_JAR_PATH: "/tmp/{{.ANTLR_JAR_NAME}}" ANTLR_DOWNLOAD_URL: "https://www.antlr.org/download/{{.ANTLR_JAR_NAME}}"Then update:
- "curl -o /tmp/antlr-4.13.1-complete.jar https://www.antlr.org/download/antlr-4.13.1-complete.jar" + "curl -fSL -o {{.ANTLR_JAR_PATH}} {{.ANTLR_DOWNLOAD_URL}}" - | - java -jar /tmp/antlr-4.13.1-complete.jar \ + | + java -jar {{.ANTLR_JAR_PATH}} \ -Dlanguage=Cpp \ -no-listener \ -visitor \ -package kql \ …🧰 Tools
🪛 GitHub Check: lint-check (ubuntu-latest)
[failure] 94-94:
94:101 [line-length] line too long (105 > 100 characters)🪛 GitHub Actions: clp-lint
[error] 94-101: line-length linting error: line too long (105 > 100 characters)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfile.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-check (ubuntu-latest)
taskfile.yaml
[failure] 94-94:
94:101 [line-length] line too long (105 > 100 characters)
🪛 GitHub Actions: clp-lint
taskfile.yaml
[error] 94-101: line-length linting error: line too long (105 > 100 characters)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (1)
taskfile.yaml (1)
89-112: Ensure CI/workflows invoke this task when grammars change
Since the generated code is checked in, you’ll need to update the CI pipeline (e.g.,.github/workflows/ci.yaml) or makecore-generatedepend onclp-s-generate-antlrso that changes to*.g4files automatically trigger regeneration. Otherwise, developers may forget to rerun the Taskfile, leading to stale generated sources.🧰 Tools
🪛 GitHub Check: lint-check (ubuntu-latest)
[failure] 94-94:
94:101 [line-length] line too long (105 > 100 characters)🪛 GitHub Actions: clp-lint
[error] 94-101: line-length linting error: line too long (105 > 100 characters)
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cpp (1)
1-10: 🧹 Nitpick (assertive)ANTLR-generated code is consistent and correct.
This base visitor implementation follows the expected pattern with proper namespace and header usage. All four ANTLR-generated files demonstrate consistent code generation.
Consider generated code management strategy.
While these files align with the PR objective to remove Java dependencies, committing generated code can present maintenance challenges. Consider documenting the regeneration process clearly and ensuring the
clp-s-generate-antlrtask mentioned in the AI summary is well-maintained to keep generated code in sync with grammar changes.
♻️ Duplicate comments (1)
taskfile.yaml (1)
103-121: Consider implementing the variable extraction suggestion from past reviewsThe hardcoded ANTLR version and long command lines make future maintenance more difficult. A past review suggested extracting ANTLR version details into variables, which would address both the line length issues and improve maintainability.
As previously suggested, consider defining variables like
ANTLR_VERSION,ANTLR_JAR_NAME, andANTLR_DOWNLOAD_URLin thevarssection to shorten these commands and simplify future version updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
components/core/src/clp_s/search/kql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h(1 hunks)components/core/src/clp_s/search/kql/kql.cpp(0 hunks)components/core/src/clp_s/search/sql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.h(1 hunks)components/core/src/clp_s/search/sql/sql.cpp(0 hunks)taskfile.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- components/core/src/clp_s/search/kql/kql.cpp
- components/core/src/clp_s/search/sql/sql.cpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlParser.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp
🧠 Learnings (2)
components/core/src/clp_s/search/sql/CMakeLists.txt (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-19
Timestamp: 2025-05-26T18:39:51.684Z
Learning: In CMakeLists.txt files, anlowee prefers to explicitly list source files one by one rather than using file(GLOB ...) or file(GLOB_RECURSE ...) to automatically include files, maintaining consistency with other CMake files in the project.
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp:8-13
Timestamp: 2025-05-26T18:55:45.377Z
Learning: ANTLR code generation includes duplicate "using namespace antlr4;" declarations intentionally: the first comes from a general template header section, and the second is added after custom namespace declarations (e.g., "using namespace kql;") to ensure ANTLR symbols remain in scope. This is a known code generation pattern, not an error.
🧬 Code Graph Analysis (4)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (2)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (30)
AND(291-293)AND(291-291)AND(792-794)AND(792-792)OR(287-289)OR(287-287)OR(796-798)OR(796-796)NOT(245-247)NOT(245-245)NOT(800-802)NOT(800-800)DATE_LITERAL(526-528)DATE_LITERAL(526-526)DATE_LITERAL(607-609)DATE_LITERAL(607-607)LITERAL(530-532)LITERAL(530-530)LITERAL(611-613)LITERAL(611-611)LITERAL(686-688)LITERAL(686-686)LITERAL(735-737)LITERAL(735-735)LITERAL(784-786)LITERAL(784-784)LITERAL(788-790)LITERAL(788-788)RANGE_OPERATOR(518-520)RANGE_OPERATOR(518-518)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (2)
KqlLexer(154-157)KqlLexer(159-161)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (1)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (2)
SqlLexer(95-98)SqlLexer(100-102)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (3)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (2)
SqlLexer(19-19)SqlLexer(21-21)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (16)
initialize(194-200)initialize(194-194)getGrammarFileName(163-165)getGrammarFileName(163-163)getRuleNames(167-169)getRuleNames(167-167)getChannelNames(171-173)getChannelNames(171-171)getModeNames(175-177)getModeNames(175-175)getVocabulary(179-181)getVocabulary(179-179)getSerializedATN(183-185)getSerializedATN(183-183)getATN(187-189)getATN(187-187)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp (10)
getGrammarFileName(98-100)getGrammarFileName(98-98)getRuleNames(102-104)getRuleNames(102-102)getVocabulary(106-108)getVocabulary(106-106)getSerializedATN(110-112)getSerializedATN(110-110)getATN(94-96)getATN(94-94)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h (3)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (33)
AND(291-293)AND(291-291)AND(792-794)AND(792-792)OR(287-289)OR(287-287)OR(796-798)OR(796-796)NOT(245-247)NOT(245-245)NOT(800-802)NOT(800-800)DATE_LITERAL(526-528)DATE_LITERAL(526-526)DATE_LITERAL(607-609)DATE_LITERAL(607-607)LITERAL(530-532)LITERAL(530-530)LITERAL(611-613)LITERAL(611-611)LITERAL(686-688)LITERAL(686-686)LITERAL(735-737)LITERAL(735-735)LITERAL(784-786)LITERAL(784-784)LITERAL(788-790)LITERAL(788-788)RANGE_OPERATOR(518-520)RANGE_OPERATOR(518-518)KqlParser(107-107)KqlParser(109-112)KqlParser(114-116)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h (12)
context(23-23)context(25-25)context(27-27)context(29-29)context(31-31)context(33-33)context(35-35)context(37-37)context(39-39)context(41-41)context(43-43)context(45-45)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (16)
ctx(20-22)ctx(20-20)ctx(24-26)ctx(24-24)ctx(28-30)ctx(28-28)ctx(32-34)ctx(32-32)ctx(36-38)ctx(36-36)ctx(40-42)ctx(40-40)ctx(44-46)ctx(44-44)ctx(48-50)ctx(48-48)
🪛 GitHub Check: lint-check (ubuntu-latest)
taskfile.yaml
[failure] 93-93:
93:13 [quoted-strings] string value is not quoted with double quotes
[failure] 91-91:
91:13 [quoted-strings] string value is not quoted with double quotes
🪛 GitHub Actions: clp-lint
taskfile.yaml
[error] 91-91: Quoted-strings lint error: string value is not quoted with double quotes.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
🔇 Additional comments (16)
components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cpp (1)
1-10: ANTLR-generated code looks correct.This is a properly generated ANTLR visitor implementation file with the expected minimal skeleton structure. The namespace and include statements are appropriate for the KQL component.
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cpp (1)
1-10: ANTLR-generated code follows expected patterns.This base visitor implementation file is correctly generated with appropriate namespace usage and header inclusion for the KQL component.
components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cpp (1)
1-10: ANTLR-generated SQL visitor implementation is correct.This file follows the expected ANTLR-generated pattern with proper namespace usage (clp_s::search::sql) and header inclusion for the SQL component.
components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.h (1)
1-29: LGTM! Well-structured ANTLR-generated visitor interface.The generated SqlVisitor.h file follows proper ANTLR conventions with correct header guards, includes, and abstract visitor pattern implementation. The namespace
clp_s::search::sqlprovides appropriate scoping for the SQL parsing components.components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h (1)
1-28: LGTM! Proper ANTLR base visitor implementation.The SqlBaseVisitor class correctly inherits from SqlVisitor and provides the standard default implementation using
visitChildren(). This follows ANTLR conventions and allows users to extend the class while only overriding the visitor methods they need.components/core/src/clp_s/search/kql/CMakeLists.txt (1)
1-22: LGTM! Successful migration from ANTLR_TARGET to explicit file listing.The replacement of the ANTLR_TARGET macro with explicit file listing achieves the PR objective of removing Java dependencies during build. The file list is comprehensive, covering all necessary ANTLR-generated components (lexer, parser, visitor, and base visitor), and the include directory setup is correctly configured.
components/core/src/clp_s/search/sql/CMakeLists.txt (1)
1-22: LGTM! Consistent migration approach matching KQL implementation.The SQL CMakeLists.txt changes mirror the KQL implementation, successfully replacing the ANTLR_TARGET macro with explicit file listing. This maintains consistency across the codebase while achieving the PR goal of removing Java dependencies. The file list is complete and the include directory configuration is correct.
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (1)
1-54: ANTLR-generated lexer header looks well-structuredThis is a standard ANTLR 4.13.1-generated lexer header that properly defines the token enums, constructor/destructor, and standard lexer interface methods. The namespace usage is consistent and the structure follows ANTLR conventions.
components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h (1)
1-51: ANTLR-generated visitor interface is well-structuredThis is a standard ANTLR 4.13.1-generated visitor interface that properly defines pure virtual methods for visiting each parse tree node type. The interface follows ANTLR conventions with appropriate return types and parameter signatures.
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (1)
1-52: LGTM! Standard ANTLR-generated lexer header.This generated lexer header follows the expected ANTLR 4.13.1 patterns. The minimal token set (only
SPACE) suggests this is either a placeholder or a very basic SQL grammar implementation.components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (1)
1-72: LGTM! Well-formed ANTLR-generated base visitor.This generated base visitor provides the expected default implementations for all KQL grammar rules, following the standard ANTLR visitor pattern.
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (1)
1-201: LGTM! Complete ANTLR-generated KQL lexer implementation.This generated lexer correctly implements all required ANTLR patterns, including thread-safe static initialization and proper memory management. The duplicate
using namespace antlr4;declarations (lines 8 and 13) are an intentional ANTLR code generation pattern to ensure symbols remain in scope after custom namespace declarations.components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp (1)
1-171: LGTM! Minimal ANTLR-generated SQL parser implementation.This generated parser correctly implements all required ANTLR patterns. The parser appears to be a minimal implementation with only a single
startrule that matches EOF, suggesting this might be a placeholder or initial framework for future SQL parsing capabilities.components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h (1)
1-65: LGTM!The ANTLR-generated SQL parser header follows the expected structure with proper header guards, namespace declaration, and virtual destructor.
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h (1)
1-253: LGTM!The ANTLR-generated KQL parser header is well-structured with proper token definitions, rule contexts, visitor pattern support, and semantic predicates for operator precedence handling.
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (1)
1-902: LGTM!The ANTLR-generated KQL parser implementation is well-structured with proper memory management, thread-safe initialization, and correct visitor pattern implementation. The semantic predicates for operator precedence are correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/clp-s-antlr-generation.yaml(1 hunks)taskfile.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/clp-s-antlr-generation.yaml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 43-43: no new line character at the end of file
(new-line-at-end-of-file)
[error] 43-43: trailing spaces
(trailing-spaces)
taskfile.yaml
[error] 124-124: trailing spaces
(trailing-spaces)
🪛 GitHub Check: lint-check (ubuntu-latest)
.github/workflows/clp-s-antlr-generation.yaml
[failure] 43-43:
43:1 [trailing-spaces] trailing spaces
[failure] 43-43:
43:9 [new-line-at-end-of-file] no new line character at the end of file
taskfile.yaml
[failure] 124-124:
124:1 [trailing-spaces] trailing spaces
🪛 GitHub Actions: clp-lint
.github/workflows/clp-s-antlr-generation.yaml
[error] 43-43: no new line character at the end of file (new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
.github/workflows/clp-s-antlr-generation.yaml (1)
43-43: Remove trailing whitespace and add EOF newline
Trailing spaces on line 43 and missing newline at end-of-file are causing lint failures.taskfile.yaml (2)
124-124: Remove trailing whitespace
Line 124 contains trailing spaces, causing lint errors.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 124-124: trailing spaces
(trailing-spaces)
🪛 GitHub Check: lint-check (ubuntu-latest)
[failure] 124-124:
124:1 [trailing-spaces] trailing spaces🪛 GitHub Actions: clp-lint
[error] 124-124: Trailing spaces detected at line 124. (trailing-spaces)
104-123: 🛠️ Refactor suggestionEncapsulate ANTLR version and JAR path into variables
Hardcoding4.13.1and/tmp/antlr-4.13.1-complete.jarcomplicates future upgrades. Define variables likeANTLR_VERSION,ANTLR_JAR_NAME,ANTLR_JAR_PATH, andANTLR_DOWNLOAD_URLin thevars:section, then reference them incmds:to improve maintainability.vars: + ANTLR_VERSION: "4.13.1" + ANTLR_JAR_NAME: "antlr-{{.ANTLR_VERSION}}-complete.jar" + ANTLR_JAR_PATH: "/tmp/{{.ANTLR_JAR_NAME}}" + ANTLR_DOWNLOAD_URL: "https://www.antlr.org/download/{{.ANTLR_JAR_NAME}}" tasks: clp-s-generate-antlr: cmds: - - | - curl -o /tmp/antlr-4.13.1-complete.jar \ - https://www.antlr.org/download/antlr-4.13.1-complete.jar + - | + curl -o {{.ANTLR_JAR_PATH}} {{.ANTLR_DOWNLOAD_URL}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/clp-s-antlr-generation.yaml(1 hunks)taskfile.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
taskfile.yaml
[error] 124-124: trailing spaces
(trailing-spaces)
🪛 GitHub Check: lint-check (ubuntu-latest)
taskfile.yaml
[failure] 124-124:
124:1 [trailing-spaces] trailing spaces
🪛 GitHub Actions: clp-lint
taskfile.yaml
[error] 124-124: Trailing spaces detected at line 124. (trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
🔇 Additional comments (2)
.github/workflows/clp-s-antlr-generation.yaml (1)
40-42: Verify ANTLR-generated code remains up-to-date
Thetask clp-s-generate-antlr-checkstep correctly ensures generated files are in sync.taskfile.yaml (1)
89-94: Preconditions ensure required tools are present
Thepreconditionsblock correctly checks for bothcurlandjava, addressing previous lint feedback and improving fail-fast behaviour.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h (1)
1-253: 💡 Verification agent🧩 Analysis chain
Verify ANTLR runtime version compatibility
Since this code is generated by ANTLR 4.13.1, ensure the ANTLR runtime library version matches to avoid potential compatibility issues.
🏁 Script executed:
#!/bin/bash # Description: Check ANTLR runtime version in CMake configuration # Search for ANTLR version specifications in CMake files rg -A 5 -B 5 "antlr|ANTLR" --glob "CMakeLists.txt" --glob "*.cmake" # Check if there's a specific version requirement for antlr4-runtime fd -e cmake -e txt -x grep -l "antlr4" {} \; | xargs grep -A 3 -B 3 "VERSION\|version"Length of output: 23206
Pin ANTLR4 runtime to v4.13.1
The CMake setup currently pulls the ANTLR C++ runtime from the master branch by default, but your grammars are generated with ANTLR 4.13.1. Mismatched versions can introduce subtle breakages.
Please update your build to lock the runtime to v4.13.1, for example:
- In components/core/cmake/Modules/ExternalAntlr4Cpp.cmake, before the
if(NOT DEFINED ANTLR4_TAG)block, add:set(ANTLR4_TAG v4.13.1 CACHE STRING "ANTLR4 runtime tag matching generated code")- Or in your top-level CMakeLists.txt, pass:
when invoking-DANTLR4_TAG=v4.13.1cmake.This ensures the ANTLR4 C++ runtime you build exactly matches your ANTLR 4.13.1-generated parsers.
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (1)
15-902: 💡 Verification agent🧩 Analysis chain
The ANTLR-generated parser implementation looks correct.
The file follows standard ANTLR 4.13.1 patterns with proper static initialization, thread-safe setup, and visitor pattern support. This aligns with the PR objective of embedding generated parser code to eliminate Java/ANTLR dependencies at build time.
Run the following script to verify the generated files are consistent:
🏁 Script executed:
#!/bin/bash # Description: Verify ANTLR-generated files are complete and consistent # Check for all expected ANTLR-generated files for KQL echo "Checking for complete set of KQL ANTLR files..." fd -e cpp -e h . components/core/src/clp_s/search/kql/antlr_generated/ --exec echo "Found: {}" # Verify the grammar file exists echo -e "\nChecking for source grammar file..." fd "Kql.g4" components/core/src/clp_s/search/kql/ # Check for generation task in taskfile echo -e "\nChecking for ANTLR generation task..." rg -A 5 "antlr.*kql" taskfile.yaml || rg -A 5 "generate.*kql" taskfile.yamlLength of output: 1221
Verification results: KQL ANTLR files present, but no generation task found
- Confirmed ANTLR-generated sources under
components/core/src/clp_s/search/kql/antlr_generated/:
• KqlVisitor.cpp/.h
• antlr4-runtime.h
• KqlParser.cpp/.h
• KqlLexer.cpp/.h
• KqlBaseVisitor.cpp/.h- Verified the grammar file exists at
components/core/src/clp_s/search/kql/Kql.g4.- No
antlr.*kqlorgenerate.*kqlentry found intaskfile.yaml.Please add or update your build/CI configuration (e.g. Taskfile, CMake, Makefile) to include a step that regenerates these parser sources from
Kql.g4, or document the manual workflow for future contributors.
♻️ Duplicate comments (3)
taskfile.yaml (3)
95-99: 🛠️ Refactor suggestionRemove generated outputs from
sourcesIncluding
antlr_generated/**/*insourceswill trigger regeneration when outputs change. It's better to list only the.g4grammar files undersourcesand rely on thegeneratesfield to track outputs.Apply this diff to fix the sources configuration:
sources: - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/kql/Kql.g4" - - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/kql/antlr_generated/**/*" - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/sql/Sql.g4" - - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/sql/antlr_generated/**/*"
105-126: 🛠️ Refactor suggestionEncapsulate ANTLR version and JAR path into variables.
Hardcoding
4.13.1and/tmp/antlr-4.13.1-complete.jarmakes future upgrades error-prone and creates long command lines.Consider defining in
vars::ANTLR_VERSION: "4.13.1" ANTLR_JAR_NAME: "antlr-{{.ANTLR_VERSION}}-complete.jar" ANTLR_JAR_PATH: "/tmp/{{.ANTLR_JAR_NAME}}" ANTLR_DOWNLOAD_URL: "https://www.antlr.org/download/{{.ANTLR_JAR_NAME}}"Then update your
curl,java -jar, andrmcommands to reference these variables instead of hardcoded values.
132-142: 🧹 Nitpick (assertive)Streamline generation check using
git diff --quietRather than two
git statuscommands and agrep, you can detect uncommitted changes in one step usinggit diff --quiet.Apply this diff to simplify the logic:
cmds: - - | - git status --porcelain \ - {{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/kql/antlr_generated \ - {{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/sql/antlr_generated - | - git status --porcelain \ - {{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/kql/antlr_generated \ - {{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/sql/antlr_generated \ - | grep . > /dev/null \ - && exit 1 \ - || exit 0 + git diff --quiet HEAD -- \ + {{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{kql,sql}/antlr_generated \ + || (echo "Generated ANTLR sources are out-of-date"; exit 1)This simplifies the logic and reduces noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cpp(1 hunks)components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cpp(1 hunks)components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.h(1 hunks)taskfile.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cppcomponents/core/src/clp_s/search/sql/antlr_generated/SqlParser.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cppcomponents/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp
🧠 Learnings (2)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp:8-13
Timestamp: 2025-05-26T18:55:45.377Z
Learning: ANTLR code generation includes duplicate "using namespace antlr4;" declarations intentionally: the first comes from a general template header section, and the second is added after custom namespace declarations (e.g., "using namespace kql;") to ensure ANTLR symbols remain in scope. This is a known code generation pattern, not an error.
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp:8-13
Timestamp: 2025-05-26T18:55:45.377Z
Learning: ANTLR code generation includes duplicate "using namespace antlr4;" declarations intentionally: the first comes from a general template header section, and the second is added after custom namespace declarations (e.g., "using namespace kql;") to ensure ANTLR symbols remain in scope. This is a known code generation pattern, not an error.
🧬 Code Graph Analysis (4)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (1)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (2)
KqlLexer(154-157)KqlLexer(159-161)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (1)
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (2)
SqlLexer(95-98)SqlLexer(100-102)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (2)
components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (2)
KqlLexer(21-21)KqlLexer(23-23)components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (12)
initialize(895-901)initialize(895-895)getGrammarFileName(122-124)getGrammarFileName(122-122)getRuleNames(126-128)getRuleNames(126-126)getVocabulary(130-132)getVocabulary(130-130)getSerializedATN(134-136)getSerializedATN(134-134)getATN(118-120)getATN(118-118)
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (2)
components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h (2)
ctx(20-22)ctx(20-20)components/core/src/clp_s/search/kql/kql.cpp (14)
ctx(118-122)ctx(118-118)ctx(124-144)ctx(124-124)ctx(146-153)ctx(146-146)ctx(155-163)ctx(155-155)ctx(165-169)ctx(165-165)ctx(171-173)ctx(171-171)ctx(175-189)ctx(175-175)
🪛 YAMLlint (1.37.1)
taskfile.yaml
[error] 127-127: trailing spaces
(trailing-spaces)
🪛 GitHub Check: lint-check (ubuntu-latest)
taskfile.yaml
[failure] 127-127:
127:1 [trailing-spaces] trailing spaces
🪛 GitHub Actions: clp-lint
taskfile.yaml
[error] 127-127: Trailing spaces detected at line 127 (trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (22)
components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.cpp (1)
1-10: LGTM! Properly generated ANTLR visitor implementation.This auto-generated file correctly includes the header and uses the appropriate namespace. The minimal implementation is expected for a generated visitor base class.
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.cpp (1)
1-10: LGTM! Properly generated ANTLR base visitor implementation.This auto-generated file follows the expected ANTLR generation pattern with correct header inclusion and namespace usage.
components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.cpp (1)
1-10: LGTM! Properly generated ANTLR SQL visitor implementation.This auto-generated file correctly uses the SQL namespace and follows the expected ANTLR generation structure. The implementation is consistent with the KQL counterpart files.
components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.cpp (1)
1-10: Skip review for generated file: This source is auto-generated by ANTLR 4.13.1. Manual modifications can cause inconsistencies with the grammar and future regenerations.components/core/src/clp_s/search/sql/antlr_generated/SqlVisitor.h (1)
1-29: Skip review for generated file: This header is auto-generated by ANTLR 4.13.1. Avoid editing to ensure it remains in sync with the grammar.components/core/src/clp_s/search/sql/antlr_generated/SqlBaseVisitor.h (1)
1-28: Skip review for generated file: This header is auto-generated by ANTLR 4.13.1. Any manual changes will be overwritten when regenerating the parser.components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.h (1)
1-54: Skip review for generated file: This header is auto-generated by ANTLR 4.13.1. Please do not modify to preserve alignment with the source grammar.components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.cpp (1)
1-142: Skip review for generated file: This implementation is auto-generated by ANTLR 4.13.1. To avoid conflicts and ensure correctness, refrain from manual edits.components/core/src/clp_s/search/kql/antlr_generated/KqlVisitor.h (1)
1-51: LGTM! Properly generated ANTLR visitor interface.The visitor interface is correctly generated with all necessary visitor methods for the KQL grammar rules. The structure follows standard ANTLR patterns with proper includes, namespace, and pure virtual method declarations.
components/core/src/clp_s/search/sql/antlr_generated/SqlLexer.h (1)
1-52: LGTM! Well-structured ANTLR-generated lexer header.The lexer header is correctly generated with proper class structure, token definitions, and all required ANTLR lexer methods. The static initialization pattern and memory management approach shown in the implementation are appropriate.
components/core/src/clp_s/search/kql/antlr_generated/KqlBaseVisitor.h (1)
1-72: LGTM! Correctly generated base visitor implementation.The base visitor provides appropriate default implementations for all visitor methods by delegating to
visitChildren(). This allows derived classes to selectively override only the methods they need, which is the standard ANTLR visitor pattern.components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp (1)
1-201: LGTM! Properly generated ANTLR lexer implementation.The lexer implementation follows standard ANTLR patterns correctly:
- Thread-safe static data initialization
- Proper memory management with interpreter cleanup in destructor
- The duplicate
using namespace antlr4;declarations (lines 8 and 13) are intentional ANTLR code generation artifacts- All lexer methods correctly delegate to static data structures
components/core/src/clp_s/search/sql/antlr_generated/SqlParser.cpp (3)
1-3: Generated file properly markedGood to see the ANTLR generation marker. This helps prevent accidental manual edits.
40-79: Thread-safe initialization looks correctThe static initialization uses proper thread safety mechanisms with
OnceFlagand conditional thread-local storage.
90-92: Proper memory managementThe destructor correctly deletes the allocated interpreter, preventing memory leaks.
components/core/src/clp_s/search/sql/antlr_generated/SqlParser.h (3)
1-4: Generated header properly markedThe ANTLR generation marker is present, which is good practice for generated files.
15-21: Minimal SQL grammar detectedThis parser only defines a SPACE token and a single start rule. Is this intended as a placeholder for future SQL grammar expansion?
56-59: Good documentation for static initializationThe comment clearly explains the lazy initialization behaviour, which helps future maintainers understand the initialization pattern.
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.h (3)
1-4: Generated KQL parser header properly markedThe ANTLR generation marker is present, maintaining consistency with other generated files.
15-25: Comprehensive KQL grammar implementationThe parser defines a complete set of tokens and rules for KQL parsing, including:
- Logical operators (AND, OR, NOT)
- Various literal types
- Range operators
- Proper rule hierarchy
This provides a solid foundation for KQL query parsing.
240-242: Semantic predicates for operator precedenceGood to see proper semantic predicate support for handling operator precedence in the grammar. This ensures correct parsing of complex KQL expressions.
components/core/src/clp_s/search/kql/antlr_generated/KqlParser.cpp (1)
1-14: LGTM! Standard ANTLR-generated file header.The file is properly generated from
clp_s/search/kql/Kql.g4by ANTLR 4.13.1 with correct includes and namespace declarations.
| antlr-generated-code-committed-check: | ||
| name: "antlr-generated-code-committed-check" |
There was a problem hiding this comment.
| antlr-generated-code-committed-check: | |
| name: "antlr-generated-code-committed-check" | |
| antlr-code-committed: | |
| name: "antlr-code-committed" |
| @@ -0,0 +1,42 @@ | |||
| name: "clp-s-generated-code-committed-check" | |||
There was a problem hiding this comment.
On second thought these names become too long when displayed, lol. How about
| name: "clp-s-generated-code-committed-check" | |
| name: "clp-s-generated-code-checks" |
| FLAVOUR: "text" | ||
| STORAGE_ENGINE: "clp" | ||
|
|
||
| clp-s-generate-parsers: |
There was a problem hiding this comment.
Can we move this task to be just above the core task?
| clp-s-generate-parsers: | ||
| vars: | ||
| CHECKSUM_FILE: "{{.G_BUILD_DIR}}/{{.TASK}}.md5" | ||
| OUTPUT_DIR: |
There was a problem hiding this comment.
Since this contains more than one output directory, how about OUTPUT_DIRS?
| - task: "utils:checksum:validate" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.OUTPUT_DIR}}"] |
There was a problem hiding this comment.
This won't pass the list as a list but will instead stringify the list. You'll notice that with the current code, when you run clp-s-generate-parsers repeatedly, it always seems to re-run. This means that it's not working properly.
To pass it as a list, change to:
| INCLUDE_PATTERNS: ["{{.OUTPUT_DIR}}"] | |
| INCLUDE_PATTERNS: | |
| ref: ".OUTPUT_DIR" |
| cmds: | ||
| - for: ["kql", "sql"] | ||
| cmd: |- | ||
| rm -rf "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated/*" \ |
There was a problem hiding this comment.
Bash won't interpret * inside quotes, so we need to put them outside the quotes:
| rm -rf "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated/*" \ | |
| rm -rf "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated/"* \ |
| && find "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated" \ | ||
| -type f -and -not \( -name "*.cpp" -o -name "*.h" \) \ | ||
| -exec rm {} \; | ||
| # This command must be last |
There was a problem hiding this comment.
Need to put empty line before comment blocks.
| # This command must be last | |
| # This command must be last |
| - task: "utils:checksum:compute" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.OUTPUT_DIR}}"] |
There was a problem hiding this comment.
| INCLUDE_PATTERNS: ["{{.OUTPUT_DIR}}"] | |
| INCLUDE_PATTERNS: | |
| ref: ".OUTPUT_DIR" |
| # Library names | ||
| G_FMT_LIB_NAME: "fmt" | ||
|
|
||
| # Antlr related |
There was a problem hiding this comment.
| # Antlr related | |
| # Antlr |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/clp-s-generated-code-checks.yaml(1 hunks)taskfile.yaml(2 hunks)taskfiles/deps/main.yaml(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
taskfile.yaml (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfile.yaml:132-142
Timestamp: 2025-05-27T19:54:46.662Z
Learning: When checking for uncommitted changes in generated code directories, `git status --porcelain` is more comprehensive than `git diff --quiet` because it detects both modified tracked files and new untracked files, while `git diff` only detects changes to already-tracked files.
taskfiles/deps/main.yaml (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
🪛 actionlint (1.7.7)
.github/workflows/clp-s-generated-code-checks.yaml
14-14: invalid job ID "antlr-code-committed:". job ID must start with a letter or _ and contain only alphanumeric characters, -, or _
(id)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
| rm -rf "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated/"* \ | ||
| && java -jar {{.G_ANTLR_JAR_FILE}} \ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Quote the ANTLR JAR path
Wrap the jar file variable in double quotes to guard against potential spaces or special characters:
- && java -jar {{.G_ANTLR_JAR_FILE}} \
+ && java -jar "{{.G_ANTLR_JAR_FILE}}" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated/"* \ | |
| && java -jar {{.G_ANTLR_JAR_FILE}} \ | |
| rm -rf "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/{{.ITEM}}/generated/"* \ | |
| && java -jar "{{.G_ANTLR_JAR_FILE}}" \ |
🤖 Prompt for AI Agents
In taskfile.yaml at lines 206 to 207, the path to the ANTLR JAR file is not
quoted, which can cause issues if the path contains spaces or special
characters. Fix this by wrapping the {{.G_ANTLR_JAR_FILE}} variable in double
quotes within the java -jar command to ensure the path is correctly interpreted.
| # Antlr | ||
| G_ANTLR_VERSION: "4.13.2" | ||
| G_ANTLR_JAR_FILE: "{{.G_DEPS_CORE_DIR}}/antlr-{{.G_ANTLR_VERSION}}-complete.jar" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Normalize acronym casing
Use uppercase # ANTLR for the comment header to align with the project’s acronym styling conventions.
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 23 to 25, the comment header uses
lowercase 'Antlr' which does not follow the project's acronym styling
conventions. Change the comment header to uppercase 'ANTLR' to normalize the
acronym casing.
| antlr-runtime: | ||
| internal: true | ||
| run: "once" | ||
| vars: | ||
| LIB_NAME: "antlr4-runtime" | ||
|
|
||
| # Paths |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a desc: field to the antlr-runtime task
Please add a concise description (for example, "Build and install the ANTLR C++ runtime") to explain its intent.
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 126 to 132, the antlr-runtime task is
missing a desc: field that provides a concise description of the task's purpose.
Add a desc: field with a brief explanation such as "Build and install the ANTLR
C++ runtime" to clarify the intent of this task.
| antlr-jar: | ||
| internal: true | ||
| vars: | ||
| DEP_NAME: "antlr" | ||
|
|
||
| # Paths | ||
| CHECKSUM_FILE: "{{.G_DEPS_CORE_CHECKSUMS_DIR}}/{{.DEP_NAME}}.md5" | ||
| OUTPUT_FILE: "{{.G_DEPS_CORE_DIR}}/antlr-4.13.1-complete.jar" | ||
| OUTPUT_FILE: "{{.G_ANTLR_JAR_FILE}}" | ||
| run: "once" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a desc: field to the antlr-jar task
Including a description such as "Download and checksum the ANTLR JAR" will document the task’s purpose and improve maintainability.
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 92 to 100, the antlr-jar task lacks a
desc: field describing its purpose. Add a desc: field with a clear description
like "Download and checksum the ANTLR JAR" to document the task's intent and
improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
.github/workflows/clp-s-generated-code-checks.yaml (1)
33-43: 🧹 Nitpick (assertive)Add failure message for outdated parsers
Currently the job exits with code 1 but prints no feedback. Prepend anecho "Generated ANTLR sources are out-of-date"beforeexit 1so failures are self-explanatory.
♻️ Duplicate comments (1)
taskfile.yaml (1)
204-218: 🧹 Nitpick (assertive)Quote ANTLR JAR path and surface errors
Wrap{{.G_ANTLR_JAR_FILE}}in double quotes to guard against spaces:- && java -jar {{.G_ANTLR_JAR_FILE}} \ + && java -jar "{{.G_ANTLR_JAR_FILE}}" \Additionally, consider echoing a clear message if ANTLR generation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/clp-s-generated-code-checks.yaml(1 hunks)taskfile.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
taskfile.yaml (1)
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfile.yaml:132-142
Timestamp: 2025-05-27T19:54:46.662Z
Learning: When checking for uncommitted changes in generated code directories, `git status --porcelain` is more comprehensive than `git diff --quiet` because it detects both modified tracked files and new untracked files, while `git diff` only detects changes to already-tracked files.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (6)
.github/workflows/clp-s-generated-code-checks.yaml (3)
1-7: Review workflow triggers
The workflow triggers on pull requests, pushes, and manual dispatch, which aligns with the intended CI guard for generated code.
29-32: macOS-only coreutils install
The conditional correctly limitsbrew install coreutilsto macOS. On Ubuntu,md5sumis available by default.
14-24: 🧹 Nitpick (assertive)Pin checkout action to a stable version
Rather than a specific commit SHA, consider usingactions/checkout@v3(or the latest major) to automatically receive non-breaking updates and security fixes.⛔ Skipped due to learnings
Learnt from: anlowee PR: y-scope/clp#925 File: .github/workflows/clp-s-antlr-generation.yaml:24-27 Timestamp: 2025-05-27T20:04:51.498Z Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.taskfile.yaml (3)
219-225: Checksum compute block looks good
The finalutils:checksum:computeinvocation correctly snapshotsOUTPUT_DIRS.
240-243: Ensure parser generation before core
Addingclp-s-generate-parserstocore-generate’s deps ensures generated sources are ready for CMake. This change aligns with the PR objective.
206-207:Details
✅ Verification successful
Verify
G_ANTLR_JAR_FILEis defined
Confirm that thedeps:antlr-jartask exposesG_ANTLR_JAR_FILE; otherwise thejava -jarinvocation will fail.
🏁 Script executed:
#!/bin/bash # Check where G_ANTLR_JAR_FILE is set rg -n 'G_ANTLR_JAR_FILE' taskfiles/deps/main.yamlLength of output: 181
G_ANTLR_JAR_FILEis defined
The variable is set intaskfiles/deps/main.yaml(line 25) toG_ANTLR_JAR_FILE: "{{.G_DEPS_CORE_DIR}}/antlr-{{.G_ANTLR_VERSION}}-complete.jar"so the
java -jar {{.G_ANTLR_JAR_FILE}}invocation will succeed.
| concurrency: | ||
| group: "${{github.workflow}}-${{github.ref}}" | ||
| # Cancel in-progress jobs for efficiency | ||
| cancel-in-progress: true | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Sanitise concurrency group
Using github.ref may include slashes (refs/heads/...), which can cause unexpected grouping. Consider using github.head_ref for PRs or replacing / with _ to ensure valid group IDs.
🤖 Prompt for AI Agents
In .github/workflows/clp-s-generated-code-checks.yaml around lines 8 to 12, the
concurrency group uses github.ref which can contain slashes, causing invalid
group IDs. Modify the concurrency group to sanitize github.ref by replacing
slashes with underscores or use github.head_ref for pull requests to ensure the
group ID is valid and consistent.
| - name: "Install task" | ||
| shell: "bash" | ||
| run: "npm install -g @go-task/cli" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Cache Task CLI install
Installing @go-task/cli on every run can slow CI. You could cache the npm global folder or use GitHub’s tool cache for faster workflows.
🤖 Prompt for AI Agents
In .github/workflows/clp-s-generated-code-checks.yaml around lines 25 to 28, the
workflow installs @go-task/cli globally on every run, which slows down the CI
process. Modify the workflow to cache the npm global folder or use GitHub
Actions' built-in tool cache to store the installed @go-task/cli between runs,
so the installation step can be skipped if the cache is valid, improving
workflow speed.
| - "deps:antlr-jar" | ||
| - task: "utils:checksum:validate" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: | ||
| ref: ".OUTPUT_DIRS" | ||
| generates: ["{{.CHECKSUM_FILE}}"] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify dependency syntax
You mix string ("deps:antlr-jar") and task: notation. For consistency, prefer:
deps:
- task: "deps:antlr-jar"
- task: "utils:checksum:validate"This aligns with other tasks and avoids ambiguity.
🤖 Prompt for AI Agents
In taskfile.yaml around lines 192 to 198, the dependency syntax is inconsistent,
mixing a string "deps:antlr-jar" with task notation. To fix this, unify the
syntax by changing the dependencies to use the task notation for both entries,
like using a deps list with each item as task: "deps:antlr-jar" and task:
"utils:checksum:validate". This will ensure consistency and clarity in the
dependency declarations.
| clp-s-generate-parsers: | ||
| vars: | ||
| CHECKSUM_FILE: "{{.G_BUILD_DIR}}/{{.TASK}}.md5" | ||
| OUTPUT_DIRS: | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/kql/generated" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/sql/generated" | ||
| deps: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a descriptive desc: field
The new clp-s-generate-parsers task lacks a desc: entry. Including a brief description (e.g. "Generate C++ parsers from ANTLR grammars for kql and sql") improves readability for future maintainers.
🤖 Prompt for AI Agents
In taskfile.yaml around lines 185 to 191, the new task clp-s-generate-parsers is
missing a desc: field. Add a desc: entry with a brief description such as
"Generate C++ parsers from ANTLR grammars for kql and sql" directly under the
task name to improve clarity and maintainability.
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
refactor(clp-s): Add ANTLR-generated parsers to codebase to eliminate need for code dependencies at build time.
Description
This PR is to remove the ANTLR's generator dependencies which needs Java. Instead, we directly put genearted code into the codebase.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit