refactor(syntax): in-place lexer, lexical scan, Module/Comment nodes - #573
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe lexer now supports offset-aware tokenization and header-name directives. Lexical scans provide C++ module and comment data to semantic analysis. Semantic tokens, indexing, completion, document links, preamble detection, and pragma folding now use token-based data. ChangesLexer and semantic integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7fdbc76ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/syntax/lexer.h (1)
20-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
lang_optslifetime requirement.
clang::Lexerkeeps a reference to theLangOptionsobject. The pointed-to object must therefore outlive theLexer. The NUL-termination contract is documented; add the same explicit note forlang_optsso callers do not pass a temporary.📝 Proposed doc addition
struct LexerOptions { /// Emit comment tokens instead of dropping them. bool keep_comments = false; + /// Borrowed, not copied: clang's lexer keeps a reference to it, so the + /// pointee must outlive the Lexer. Null selects a default instance. const clang::LangOptions* lang_opts = nullptr; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/syntax/lexer.h` around lines 20 - 46, Update the LexerOptions documentation for lang_opts to state that the referenced clang::LangOptions object must outlive the Lexer and must not be provided as a temporary, matching the explicit lifetime guidance style used for Lexer content.src/syntax/lexical_scan.h (1)
31-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive
ModuleDeclaration::kinda default value.
lexical_scandeclaresModuleDeclaration decl;and assignskindonly on the paths that push the record, so the current code is correct. A future path that pushes a record before assigningkindwould store an indeterminate value, andsrc/index/tu_index.cppswitches on it. A default initializer removes that risk at no cost.♻️ Proposed change
- Kind kind; + Kind kind = Kind::Declaration;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/syntax/lexical_scan.h` around lines 31 - 60, Initialize ModuleDeclaration::kind with a safe default enum value at its declaration, preserving the existing assignments in lexical_scan and ensuring any record pushed before explicit assignment has a determinate kind for consumers such as the switch in tu_index.cpp.src/semantic/semantics.h (1)
377-380: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState the mutation invariant next to the storage.
Move-only construction protects the payload pointers against copies, and moving the two
std::vectormembers keeps element addresses stable. The remaining hazard is a laterpush_backoreraseonlexical.commentsorlexical.modulesafterappend_directivesrecorded pointers into them.SemanticsBuilderis the only friend, so this is enforceable today; record the invariant so it stays that way.📝 Proposed doc addition
/// The lexically scanned entities; Module and Comment nodes point into - /// this storage. + /// this storage. Both vectors must not be resized after the nodes are + /// built (the builder scans and filters before it takes any pointer). LexicalInfo lexical;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/semantic/semantics.h` around lines 377 - 380, Add documentation next to the LexicalInfo lexical storage member stating that after append_directives records payload pointers, lexical.comments and lexical.modules must not be mutated with push_back, erase, or equivalent operations. Note that SemanticsBuilder is responsible for preserving this invariant and keep the existing storage declaration unchanged.src/syntax/lexer.cpp (1)
83-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the comment:
export importalso reaches this branch.Line 71 sets
parse_pp_keyword = trueforexportat the start of a line whilemodule_declaration_contextholds. The next token then enters this branch. Forexport import <vector>;orexport import "hdr.h";the header-name mode is in fact what you want, and a plain module name still lexes as an identifier. So the behavior is fine, but the claim "a module import never sets parse_pp_keyword" is inaccurate and will mislead the next reader.📝 Proposed comment fix
- // `import` here is the directive form (`#import`), which takes a - // filename; a module import never sets parse_pp_keyword. + // `import` here is either the directive form (`#import`) or the + // `export import` opener; both take a header name when one is + // written, and a plain module name still lexes as an identifier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/syntax/lexer.cpp` around lines 83 - 95, Correct the comment above the parse_header_name assignment to acknowledge that export import can also reach this branch and that header-name mode is intentional for directive-style and exported imports, while plain module names remain identifiers. Do not change the existing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/feature/document_links.cpp`:
- Around line 29-36: Add "import" to the recognized keyword comparisons in
find_directive_argument so `#import` directives set after_keyword before locating
their filename argument. Preserve the existing handling for all other directive
keywords.
In `@tests/unit/syntax/lexer_tests.cpp`:
- Around line 152-161: In the MacroArgument test, assert that tokens contains
the expected number of entries before indexing tokens[2], matching the guard
used by the IncompleteInput cases. Also protect the tokens.back() access near
the related test by asserting the vector is non-empty before using it.
---
Nitpick comments:
In `@src/semantic/semantics.h`:
- Around line 377-380: Add documentation next to the LexicalInfo lexical storage
member stating that after append_directives records payload pointers,
lexical.comments and lexical.modules must not be mutated with push_back, erase,
or equivalent operations. Note that SemanticsBuilder is responsible for
preserving this invariant and keep the existing storage declaration unchanged.
In `@src/syntax/lexer.cpp`:
- Around line 83-95: Correct the comment above the parse_header_name assignment
to acknowledge that export import can also reach this branch and that
header-name mode is intentional for directive-style and exported imports, while
plain module names remain identifiers. Do not change the existing logic.
In `@src/syntax/lexer.h`:
- Around line 20-46: Update the LexerOptions documentation for lang_opts to
state that the referenced clang::LangOptions object must outlive the Lexer and
must not be provided as a temporary, matching the explicit lifetime guidance
style used for Lexer content.
In `@src/syntax/lexical_scan.h`:
- Around line 31-60: Initialize ModuleDeclaration::kind with a safe default enum
value at its declaration, preserving the existing assignments in lexical_scan
and ensuring any record pushed before explicit assignment has a determinate kind
for consumers such as the switch in tu_index.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05c94cc1-d07c-4620-95e0-e532f55270ce
📒 Files selected for processing (32)
docs/en/features/folding-ranges.mdsrc/compile/directive.cppsrc/feature/document_links.cppsrc/feature/semantic_tokens.cppsrc/index/tu_index.cppsrc/semantic/semantics.cppsrc/semantic/semantics.hsrc/syntax/completion.cppsrc/syntax/lexer.cppsrc/syntax/lexer.hsrc/syntax/lexical_scan.cppsrc/syntax/lexical_scan.hsrc/syntax/scan.cpptests/snap/folding_range/abbreviated_function_template.cpptests/snap/folding_range/comment_folding.cpptests/snap/folding_range/coroutine_body.cpptests/snap/folding_range/include_region.cpptests/snap/folding_range/initializer_list_construction.cpptests/snap/folding_range/macro_folding.cpptests/snap/folding_range/pragma_classification.cpptests/snap/folding_range/pragma_classification.snap.ymltests/snap/folding_range/raw_string_literal.cpptests/snap/folding_range/template_instantiations.cpptests/snap/folding_range/template_parameter_list.cpptests/snap/folding_range/using_declaration_block.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/semantic/semantics_tests.cpptests/unit/syntax/completion_tests.cpptests/unit/syntax/lexer_tests.cpptests/unit/syntax/lexical_scan_tests.cpptests/unit/syntax/scan_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 987286396f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7868d39db1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a664a55f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
This round refactors the lexing layer and gives the semantics table first-class coverage of everything the AST and preprocessor callbacks fail to record.
Lexer
Lexer::from_line(content, offset)starts at the line containingoffset; token ranges stay in file coordinates, so they compose directly with feature offsets. The hand-writtenrfind('\n') → substr → rebasedance at call sites is gone.LexerOptions; the deadignore_end_of_directiveflag and the unusedis_directive_keywordhelper removed.__has_include/__has_embedfamily (mode switches after the opening paren) and the#importdirective; the residual "pick the filename argument" grammar moved next to its only consumer (document links).lexical_scan (new)
One pass over the file collects what neither the AST nor PPCallbacks report, as structured data: comments (range + line/block kind) and the three module-declaration forms (
module;,[export] module x.y:z;,module :private;) with per-token ranges.Semantics table: Module and Comment nodes
New
ModuleandCommentnode kinds are fed by the lexical scan at build time, cross-checked against the compiler before becoming nodes: named-module gate, DefinitionLoc anchor for the declaration form (macro-spelled names survive, disabled-branch duplicates die), spelled-token liveness for the private fragment. Module nodes own their written tokens like other directive nodes.Consumers then deleted their own scanners:
Fixes along the way
#pragma region/endregionis classified by the first argument token instead of substring matching — a pragma merely mentioning "endregion" no longer closes a fold early (pinned by a new folding fixture).is_preamble_complete: a trailing comment no longer hides a terminating semicolon (or fakes one), and angled#import <...>completes.detect_completion_contextrewritten on the lexer: comment-tolerant, exact keyword boundaries, and the cursor-at-line-start case no longer leaks the previous line.Import-channel audit
The preprocessor callback channel and the AST
ImportDeclchannel agree on all three import forms (named, export-import, partition); pinned by a unit test. The preamble-PCH boundary remains the shared, by-design gap handled per feature by PreambleState.Tests
module, strings, comments, disabled branches), semantics-table nodes and gates, module painting under a real PCH split, partition and implementation-unit indexing, preamble/completion edge cases including CRLF and cursor-mid-keyword.