refactor: public feature types and snapshot testing infrastructure - #442
Merged
16bit-ykiko merged 8 commits intoMay 24, 2026
Merged
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:
📝 WalkthroughWalkthroughThis PR refactors semantic feature pipelines to eliminate intermediate data types by collecting directly into final ChangesSemantic Feature Refactoring and Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@tests/snapshots/document_symbol/snapshot/statements/if/basic_if.cpp.snap.yml`:
- Around line 7-13: The snapshot file contains invalid YAML because a block
sequence (the child entries for the "test" function) directly follows a
flow-style mapping; update the snapshot so the "test" function entry uses an
explicit children: key and block-style expansion for its nested symbols instead
of placing child mappings directly after the parent mapping; locate the "test"
function entry (selection_range "29:5-29:9") and replace the inline child
sequence for r1, r2, r3 with a children: block containing those variables,
ensuring other top-level function entries (abs_val, sign, nested_if) remain
unchanged.
In `@tests/unit/test/tester.cpp`:
- Around line 239-241: The code currently strips directory context by using
llvm::sys::path::filename(path) before calling add_main and compile, which
breaks relative include resolution; update the call to preserve the original
file path (use path instead of filename) when invoking add_main (and keep
passing (*buffer)->getBuffer()) so the compile function receives the full path
context for correct include resolution and snapshot results.
🪄 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
Run ID: f94a9929-5512-4dd2-8e34-a338b30300d2
📒 Files selected for processing (27)
cmake/package.cmakepixi.tomlsrc/feature/document_symbols.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/semantic/semantic_visitor.hsrc/server/worker/stateful_worker.cppsrc/server/workspace/config.cpptests/corpus/statements/if/basic_if.cpptests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/document_symbol/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/semantic_tokens/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/tu_index/snapshot/statements/if/basic_if.cpp.snap.ymltests/unit/feature/document_link_tests.cpptests/unit/feature/document_symbol_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/test/platform.htests/unit/test/tester.cpptests/unit/test/tester.htests/unit/unit_tests.cc
16bit-ykiko
force-pushed
the
refactor/snapshot-testing-and-public-feature-types
branch
from
May 20, 2026 15:29
a980979 to
24a468b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/feature/semantic_tokens.cpp (1)
80-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExclude file-scope
staticfunctions from theStatictoken modifier.Line 80 now treats
staticfree functions asStatic, but the contract in the comment above says namespace-scopedstaticonly changes linkage and should not get this modifier. That will skew semantic highlighting and snapshots for file-local functions.Suggested fix
- if(const auto* function = llvm::dyn_cast<clang::FunctionDecl>(decl)) { - return function->isStatic(); - } return false;🤖 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/feature/semantic_tokens.cpp` around lines 80 - 82, The current check treats any clang::FunctionDecl with function->isStatic() as the Static modifier, which incorrectly marks file-scope (namespace-scoped) static free functions; update the condition to only apply the Static modifier for static class/member functions by testing that the FunctionDecl is a C++ method (e.g., dyn_cast<clang::CXXMethodDecl> or function->isCXXClassMember()) and then checking isStatic(); change the block that uses the local variable function so it returns Static only when the decl is a CXXMethodDecl and isStatic() is true.
🤖 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 `@tests/unit/feature/document_symbol_tests.cpp`:
- Around line 199-220: The current serialization in format_document_symbols
emits child nodes directly after a flow-style mapping for a node, producing
invalid YAML; modify format_document_symbols so that when node.children is
non-empty it first appends an explicit block key (e.g. "children:") at the
current indentation level and then serializes the child nodes in block style by
calling format_document_symbols for node.children with increased
depth/indentation (instead of writing a raw nested sequence after the flow
mapping); ensure the "children" line uses the same pad/indent logic and that
child nodes are emitted as block entries so yaml_str(node.name)/detail and
selection_range remain inside the parent mapping correctly.
---
Outside diff comments:
In `@src/feature/semantic_tokens.cpp`:
- Around line 80-82: The current check treats any clang::FunctionDecl with
function->isStatic() as the Static modifier, which incorrectly marks file-scope
(namespace-scoped) static free functions; update the condition to only apply the
Static modifier for static class/member functions by testing that the
FunctionDecl is a C++ method (e.g., dyn_cast<clang::CXXMethodDecl> or
function->isCXXClassMember()) and then checking isStatic(); change the block
that uses the local variable function so it returns Static only when the decl is
a CXXMethodDecl and isStatic() is true.
🪄 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
Run ID: 2e6dbea4-dd5e-4092-919f-d21139cfc90c
📒 Files selected for processing (26)
cmake/package.cmakepixi.tomlsrc/feature/document_symbols.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/worker/stateful_worker.cppsrc/server/workspace/config.cpptests/corpus/statements/if/basic_if.cpptests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/document_symbol/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/semantic_tokens/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/tu_index/snapshot/statements/if/basic_if.cpp.snap.ymltests/unit/feature/document_link_tests.cpptests/unit/feature/document_symbol_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/test/platform.htests/unit/test/tester.cpptests/unit/test/tester.htests/unit/unit_tests.cc
✅ Files skipped from review due to trivial changes (4)
- tests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/tu_index/snapshot/statements/if/basic_if.cpp.snap.yml
16bit-ykiko
force-pushed
the
refactor/snapshot-testing-and-public-feature-types
branch
2 times, most recently
from
May 21, 2026 03:15
ef6b23c to
bd8a173
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/feature/document_symbol_tests.cpp (1)
199-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit nested symbols under an explicit
childrenfield.Current serialization writes child entries directly after a flow mapping, which breaks YAML structure for nested nodes.
Proposed fix
- out += std::format("{}- {{name: {}, kind: {}, range: \"{}:{}-{}:{}\"", - pad, - yaml_str(node.name), - kind, - start->line, - start->character, - end->line, - end->character); - if(sel_start && sel_end) { - out += std::format(", selection_range: \"{}:{}-{}:{}\"", - sel_start->line, - sel_start->character, - sel_end->line, - sel_end->character); - } - if(!node.detail.empty()) { - out += std::format(", detail: {}", yaml_str(node.detail)); - } - out += "}\n"; - if(!node.children.empty()) { - format_document_symbols(out, mapper, node.children, depth + 1); - } + out += std::format("{}- name: {}\n", pad, yaml_str(node.name)); + out += std::format("{} kind: {}\n", pad, kind); + out += std::format("{} range: \"{}:{}-{}:{}\"\n", + pad, + start->line, + start->character, + end->line, + end->character); + if(sel_start && sel_end) { + out += std::format("{} selection_range: \"{}:{}-{}:{}\"\n", + pad, + sel_start->line, + sel_start->character, + sel_end->line, + sel_end->character); + } + if(!node.detail.empty()) { + out += std::format("{} detail: {}\n", pad, yaml_str(node.detail)); + } + if(!node.children.empty()) { + out += std::format("{} children:\n", pad); + format_document_symbols(out, mapper, node.children, depth + 1); + }🤖 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 `@tests/unit/feature/document_symbol_tests.cpp` around lines 199 - 220, The serialization currently closes the node mapping before emitting children; move child output into an explicit "children" field so nested symbols are valid YAML: inside format_document_symbols (the function building `out`), when node.children is non-empty, append ", children: [" (instead of closing the "}" first), then recurse into format_document_symbols to emit child entries, and finally append the closing "]}" (or corresponding closing mapping characters) and newline; ensure you still emit selection_range and detail into the parent mapping before starting the children array and reference `node.children`, `yaml_str`, and `out` to locate the changes.
🤖 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.
Duplicate comments:
In `@tests/unit/feature/document_symbol_tests.cpp`:
- Around line 199-220: The serialization currently closes the node mapping
before emitting children; move child output into an explicit "children" field so
nested symbols are valid YAML: inside format_document_symbols (the function
building `out`), when node.children is non-empty, append ", children: ["
(instead of closing the "}" first), then recurse into format_document_symbols to
emit child entries, and finally append the closing "]}" (or corresponding
closing mapping characters) and newline; ensure you still emit selection_range
and detail into the parent mapping before starting the children array and
reference `node.children`, `yaml_str`, and `out` to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcca24a6-78e5-424b-8ef8-5e856e6a56ae
📒 Files selected for processing (26)
cmake/package.cmakepixi.tomlsrc/feature/document_symbols.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/worker/stateful_worker.cppsrc/server/workspace/config.cpptests/corpus/statements/if/basic_if.cpptests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/document_symbol/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/semantic_tokens/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/tu_index/snapshot/statements/if/basic_if.cpp.snap.ymltests/unit/feature/document_link_tests.cpptests/unit/feature/document_symbol_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/test/platform.htests/unit/test/tester.cpptests/unit/test/tester.htests/unit/unit_tests.cc
✅ Files skipped from review due to trivial changes (4)
- tests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.yml
- cmake/package.cmake
…infrastructure Move internal result types (SemanticToken, FoldingRange, DocumentSymbol, InlayHint, HintCategory) from .cpp files into feature.h as public types. Add overloaded APIs: raw overloads (no encoding) return internal types, protocol overloads (with encoding) return LSP types. Wire up snapshot testing with ASSERT_SNAPSHOT_GLOB, YAML format functions, and --snapshot-dir/--update-snapshots CLI flags. Improve SemanticVisitor with LabelStmt/GotoStmt visitors, unnamed template param guards, and InjectedClassNameTypeLoc support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
refactor/snapshot-testing-and-public-feature-types
branch
from
May 21, 2026 06:40
bd8a173 to
9518ffb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/feature/semantic_tokens.cpp (1)
455-509:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack the start column of the last emitted segment.
After a multiline token,
last_start_characteris reset to the originalbegin_char, but the last emitted piece onend_linestarts at column0. The next token on that line gets a wrongdelta_start, which breaks semantic token encoding after block comments/raw strings.Suggested fix
- last_line = end_line; - last_start_character = begin_char; + last_line = end_line; + last_start_character = begin_line == end_line ? begin_char : 0;🤖 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/feature/semantic_tokens.cpp` around lines 455 - 509, The append method resets last_start_character incorrectly after multi-line tokens (it uses the original begin_char), causing wrong delta_start for the next token on the same line; change the update so that after emitting a multi-line token you set last_start_character to the start column of the last emitted piece (which is 0 for the final piece on the end_line) instead of begin_char — i.e. update the assignment of last_start_character (near the end of append) to conditional behavior: if begin_line == end_line keep begin_char, otherwise set last_start_character to 0 (ensure this happens after emitting the final piece and before updating last_line).
♻️ Duplicate comments (2)
tests/unit/feature/document_symbol_tests.cpp (1)
199-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit valid nested YAML for child document symbols.
Current output writes child
-entries after a flow mapping parent, which is invalid YAML. Serialize parent nodes in block style and add an explicitchildren:key before recursive output.Proposed fix
- out += std::format("{}- {{name: {}, kind: {}, range: \"{}:{}-{}:{}\"", - pad, - yaml_str(node.name), - kind, - start->line, - start->character, - end->line, - end->character); + out += std::format("{}- name: {}\n", pad, yaml_str(node.name)); + out += std::format("{} kind: {}\n", pad, kind); + out += std::format("{} range: \"{}:{}-{}:{}\"\n", + pad, + start->line, + start->character, + end->line, + end->character); if(sel_start && sel_end) { - out += std::format(", selection_range: \"{}:{}-{}:{}\"", + out += std::format("{} selection_range: \"{}:{}-{}:{}\"\n", + pad, sel_start->line, sel_start->character, sel_end->line, sel_end->character); } if(!node.detail.empty()) { - out += std::format(", detail: {}", yaml_str(node.detail)); + out += std::format("{} detail: {}\n", pad, yaml_str(node.detail)); } - out += "}\n"; if(!node.children.empty()) { + out += std::format("{} children:\n", pad); format_document_symbols(out, mapper, node.children, depth + 1); }🤖 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 `@tests/unit/feature/document_symbol_tests.cpp` around lines 199 - 220, The current formatter in format_document_symbols writes parent nodes as a flow mapping (using "{...}") and then emits child "- " entries, producing invalid YAML; change format_document_symbols to emit the parent as a block mapping (each field on its own indented line) rather than a flow mapping, write a "children:" key (with correct indentation using pad) before recursing, and then call format_document_symbols for node.children; preserve existing fields (name, kind, range, optional selection_range and detail) but output them as block entries instead of inside "{...}" so nested "- " child entries are valid YAML.tests/unit/test/tester.cpp (1)
239-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the full source path in
compile_file.Using only the basename loses directory context. That can break relative include resolution and distort snapshot behavior for corpus files in nested directories.
Proposed fix
- auto filename = llvm::sys::path::filename(path); - add_main(filename, (*buffer)->getBuffer()); + add_main(path, (*buffer)->getBuffer());🤖 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 `@tests/unit/test/tester.cpp` around lines 239 - 240, The code in compile_file strips directory context by calling llvm::sys::path::filename(path) and passing that to add_main; preserve the full source path instead: pass the full path (not the basename) into add_main so relative includes and snapshot/corpus behavior remain correct (e.g., replace the use of filename with the original path string when calling add_main in compile_file and adjust any variable name accordingly).
🧹 Nitpick comments (1)
src/feature/inlay_hints.cpp (1)
900-910: 🏗️ Heavy liftHonor
targetbefore walking the whole TU.
targetonly filters hints at insertion time; Line 910 still traverses and analyzes the full AST for every request. For visible-range inlay-hint queries, that keeps latency proportional to file size instead of the requested window.🤖 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/feature/inlay_hints.cpp` around lines 900 - 910, The inlay_hints function currently always calls visitor.TraverseDecl(unit.tu()), causing the entire TU to be analyzed even when a small LocalSourceRange target was requested; modify inlay_hints (and/or the Visitor) so you first detect whether target is a visible-range window and, when so, either iterate only the top-level declarations that intersect target or make Visitor prune traversal for AST nodes whose source ranges lie fully outside target; update Builder/Visitor constructors (Builder(raw_hints, unit, target, options) and Visitor(visitor, unit, target, options)) usage so traversal is limited to nodes overlapping target instead of always calling TraverseDecl(unit.tu()). Ensure the logic references the LocalSourceRange target when deciding to skip or descend into nodes.
🤖 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 `@tests/unit/index/tu_index_tests.cpp`:
- Line 513: The current sort call on the vector named "sorted" uses a key
extractor that returns only o.range.begin, causing non-deterministic ordering
for entries with equal starts; change the comparator/key to produce a
deterministic tie (e.g., compare by o.range.begin then o.range.end, or return a
tuple/tie of begin and end) so equal start offsets are ordered
consistently—update the std::ranges::sort invocation (the lambda currently
returning o.range.begin) to use begin+end tie comparison or a two-field
comparator for stable, deterministic ordering.
---
Outside diff comments:
In `@src/feature/semantic_tokens.cpp`:
- Around line 455-509: The append method resets last_start_character incorrectly
after multi-line tokens (it uses the original begin_char), causing wrong
delta_start for the next token on the same line; change the update so that after
emitting a multi-line token you set last_start_character to the start column of
the last emitted piece (which is 0 for the final piece on the end_line) instead
of begin_char — i.e. update the assignment of last_start_character (near the end
of append) to conditional behavior: if begin_line == end_line keep begin_char,
otherwise set last_start_character to 0 (ensure this happens after emitting the
final piece and before updating last_line).
---
Duplicate comments:
In `@tests/unit/feature/document_symbol_tests.cpp`:
- Around line 199-220: The current formatter in format_document_symbols writes
parent nodes as a flow mapping (using "{...}") and then emits child "- "
entries, producing invalid YAML; change format_document_symbols to emit the
parent as a block mapping (each field on its own indented line) rather than a
flow mapping, write a "children:" key (with correct indentation using pad)
before recursing, and then call format_document_symbols for node.children;
preserve existing fields (name, kind, range, optional selection_range and
detail) but output them as block entries instead of inside "{...}" so nested "-
" child entries are valid YAML.
In `@tests/unit/test/tester.cpp`:
- Around line 239-240: The code in compile_file strips directory context by
calling llvm::sys::path::filename(path) and passing that to add_main; preserve
the full source path instead: pass the full path (not the basename) into
add_main so relative includes and snapshot/corpus behavior remain correct (e.g.,
replace the use of filename with the original path string when calling add_main
in compile_file and adjust any variable name accordingly).
---
Nitpick comments:
In `@src/feature/inlay_hints.cpp`:
- Around line 900-910: The inlay_hints function currently always calls
visitor.TraverseDecl(unit.tu()), causing the entire TU to be analyzed even when
a small LocalSourceRange target was requested; modify inlay_hints (and/or the
Visitor) so you first detect whether target is a visible-range window and, when
so, either iterate only the top-level declarations that intersect target or make
Visitor prune traversal for AST nodes whose source ranges lie fully outside
target; update Builder/Visitor constructors (Builder(raw_hints, unit, target,
options) and Visitor(visitor, unit, target, options)) usage so traversal is
limited to nodes overlapping target instead of always calling
TraverseDecl(unit.tu()). Ensure the logic references the LocalSourceRange target
when deciding to skip or descend into nodes.
🪄 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
Run ID: 0b9209e6-aa91-488a-b009-a0907658fddb
📒 Files selected for processing (26)
cmake/package.cmakepixi.tomlsrc/feature/document_symbols.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/worker/stateful_worker.cppsrc/server/workspace/config.cpptests/corpus/statements/if/basic_if.cpptests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/document_symbol/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/semantic_tokens/snapshot/statements/if/basic_if.cpp.snap.ymltests/snapshots/tu_index/snapshot/statements/if/basic_if.cpp.snap.ymltests/unit/feature/document_link_tests.cpptests/unit/feature/document_symbol_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/test/platform.htests/unit/test/tester.cpptests/unit/test/tester.htests/unit/unit_tests.cc
✅ Files skipped from review due to trivial changes (6)
- tests/snapshots/document_link/snapshot/statements/if/basic_if.cpp.snap.yml
- cmake/package.cmake
- tests/snapshots/inlay_hint/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/folding_range/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/semantic_tokens/snapshot/statements/if/basic_if.cpp.snap.yml
- tests/snapshots/tu_index/snapshot/statements/if/basic_if.cpp.snap.yml
… cc1 args Update kotatsu to 441856c (merged PR #141) which unifies RunnerOptions and ZestCliOptions into a single Options struct with deco decorators. Rewrite unit_tests.cc to use composition pattern embedding kota::zest::Options. Filter unknown cc1 flags from system clang when its version is newer than our embedded LLVM, preventing unrecognized argument errors at startup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prettier reformats .snap.yml files (adding spaces inside braces, stripping trailing newlines) which breaks snapshot comparison. Exclude *.snap.yml from the format-yaml task to keep snapshot files byte-exact. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Includes fix for simdjson string buffer overflow on variant try_read with checkpoint/restore of full json_iterator state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use (begin, end, target) tuple comparison to match production sort order and prevent flaky snapshot diffs from equal start offsets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
changed the title
Inline single-use format functions into snapshot lambdas, remove redundant clear() calls, add spaces inside YAML flow mappings, fix document_symbol indentation to produce valid YAML, and drop document_link snapshot tests (not meaningful for generic corpus files). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…encoding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
changed the title
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
deleted the
refactor/snapshot-testing-and-public-feature-types
branch
May 24, 2026 11:36
3 tasks done
This was referenced Jul 12, 2026
This was referenced Jul 19, 2026
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SemanticToken,FoldingRange,DocumentSymbol,InlayHint, andHintCategoryfrom internal.cppfiles tofeature.has public API types. Each feature now exposes two overloads: a raw overload returning offset-based types and a protocol overload that converts to LSP wire-format with explicitPositionEncoding.ASSERT_SNAPSHOT_GLOBfor semantic tokens, folding ranges, inlay hints, document symbols, and TU index. Tests compile real C++ corpus files, format output as YAML flow mappings, and diff against.snap.ymlbaselines.compile_file()toTester,yaml_str()utility,--corpus-dir/--snapshot-dirCLI options, and--verboseflag for unit tests. Migrate to kotatsu's unifiedkota::zest::OptionsAPI.clang::driver::getDriverOptTable()to handle system compilers newer than embedded LLVM.from_jsonAPI).Details
Feature type changes
All five feature modules (
semantic_tokens,folding_ranges,document_symbols,inlay_hints,document_links) now follow the same two-overload pattern. The raw overload returns offset-based structs suitable for indexing and testing; the protocol overload addsPositionEncodingconversion for LSP responses.stateful_worker.cppexplicitly passesPositionEncoding::UTF16at every call site.Snapshot tests
Corpus files live in
tests/corpus/(organized by language construct). Snapshot baselines live intests/snapshots/<feature>/. Format lambdas are inlined directly in test bodies — no separate format functions for single-use formatters. YAML output uses flow mappings (- { key: value }) for compact, diffable baselines.cc1 arg filtering
src/command/toolchain.cppnow parses the cc1 argument list through LLVM's driver option table and drops any args classified asUnknownClass. This prevents compilation failures when the system compiler emits flags that the embedded LLVM version doesn't recognize.🤖 Generated with Claude Code