Repository navigation
feat(hover): initial support for hover - #440
Closed
16bit-ykiko wants to merge 3 commits into
Closed
16bit-ykiko wants to merge 3 commits into
16bit-ykiko wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughReplaces simple hover markdown with a structured ChangesHover Feature & Markup Foundation
Markup Refactor & Tests
Formatting API
Sequence DiagramsequenceDiagram
actor User
participant HoverEngine as Hover Engine
participant TokenInspector as Token Inspector
participant ASTResolver as AST Resolver
participant HoverBuilder as HoverInfo Builder
participant MarkupRenderer as Markup Renderer
participant Protocol as Protocol
User->>HoverEngine: Request hover at offset
HoverEngine->>TokenInspector: Inspect tokens (macro / auto / decltype)
alt Macro found
TokenInspector-->>HoverEngine: Macro token
HoverEngine->>HoverBuilder: hover_macro()
else Deduced type found
TokenInspector-->>HoverEngine: Deduced-type location
HoverEngine->>ASTResolver: get_deduced_type()
ASTResolver-->>HoverEngine: Resolved QualType
HoverEngine->>HoverBuilder: hover_deduced_type()
else No token match
HoverEngine->>ASTResolver: Selection-tree lookup
ASTResolver-->>HoverEngine: NamedDecl or Expression
HoverEngine->>HoverBuilder: hover_decl() / expression analysis
end
HoverBuilder->>HoverBuilder: Populate scopes, types, params, docs, layout, value
HoverBuilder-->>HoverEngine: HoverInfo
HoverEngine->>MarkupRenderer: present(HoverInfo)
MarkupRenderer->>MarkupRenderer: Build Markup document
MarkupRenderer-->>HoverEngine: Markdown string
HoverEngine->>Protocol: Build protocol::Hover (content + range)
Protocol-->>User: Hover response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/support/markup.cpp (1)
75-87:⚠️ Potential issue | 🟠 MajorCast before calling
std::isspace().These
std::isspace()calls operate on plaincharbytes. That is undefined behavior for negative values, and this renderer already sees UTF-8 input, so non-ASCII hover text can misbehave here.Suggested fix
Paragraph& Paragraph::append_text(std::string text, Kind kind) { + auto is_space = [](char ch) { + return std::isspace(static_cast<unsigned char>(ch)) != 0; + }; if(kind == Kind::PlainText) { llvm::StringRef s{text}; if(s.empty()) { return *this; } - bool flag = !chunks.empty() && !std::isspace(chunks.back().content.back()); + bool flag = !chunks.empty() && !is_space(chunks.back().content.back()); auto& chunk = chunks.emplace_back(); chunk.kind = Kind::PlainText; chunk.content = std::move(s.str()); chunk.space_ahead = flag; - chunk.space_after = !std::isspace(s.back()); + chunk.space_after = !is_space(s.back()); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/support/markup.cpp` around lines 75 - 87, Paragraph::append_text currently calls std::isspace on plain char values (e.g., chunks.back().content.back() and s.back()), which is undefined for negative (non-ASCII) bytes; cast those char values to unsigned char before passing to std::isspace to handle UTF-8 bytes safely. Update the checks that set flag (used for chunk.space_ahead) and chunk.space_after to call std::isspace(static_cast<unsigned char>(...)) so the logic around chunks, chunk.kind and chunk.content remains unchanged but avoids UB on non-ASCII input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/feature/hover.cpp`:
- Around line 1027-1036: The inline construction of protocol::Hover here drops
the hover span; instead call the existing build_hover(...) helper (which
preserves range/metadata) or populate result.range from the original HoverInfo
(hi) before returning. Specifically, replace the inline protocol::Hover
construction around present(*hi) with a call to build_hover(unit, hi, encoding)
(or use PositionMapper + present(*hi) but copy hi->range into result.range) so
PositionMapper, converter, protocol::Hover, present(*hi) and build_hover() are
used to preserve the range metadata.
- Around line 646-811: The present(const HoverInfo& hi) renderer currently
always strips Doxygen and emits markdown; update it to honor HoverOptions by
adding a HoverOptions const& options parameter to present (and update its
callers) and gate the Doxygen/markdown behavior: only call
strip_doxygen_info/use markdown formatting when options.enable_doxygen_parsing
and options.parse_comment_as_markdown are true respectively (leave current
behavior when those flags are true, and if false, append raw/rest text without
Doxygen parsing or markdown decoration). Ensure you reference the HoverInfo
users of strip_doxygen_info and the parts that build doc/param/return blocks so
they run conditionally based on the new HoverOptions flags.
In `@src/support/markup.cpp`:
- Around line 189-191: Markup::append currently moves pointers out of
other.blocks but leaves other.blocks the same length filled with null
unique_ptrs, causing later calls (other.as_markdown, render_blocks, or a second
append) to dereference nulls; fix by moving with proper move-iterators (e.g.,
std::make_move_iterator) into blocks and then clear or swap out other.blocks so
its size becomes zero (e.g., other.blocks.clear() or other.blocks = {};),
ensuring other.blocks contains no null entries after Markup::append.
---
Outside diff comments:
In `@src/support/markup.cpp`:
- Around line 75-87: Paragraph::append_text currently calls std::isspace on
plain char values (e.g., chunks.back().content.back() and s.back()), which is
undefined for negative (non-ASCII) bytes; cast those char values to unsigned
char before passing to std::isspace to handle UTF-8 bytes safely. Update the
checks that set flag (used for chunk.space_ahead) and chunk.space_after to call
std::isspace(static_cast<unsigned char>(...)) so the logic around chunks,
chunk.kind and chunk.content remains unchanged but avoids UB on non-ASCII input.
🪄 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: fe12b712-b4de-473b-95c1-fc7206fc556a
📒 Files selected for processing (8)
src/feature/hover.cppsrc/support/doxygen.cppsrc/support/doxygen.hsrc/support/markup.cppsrc/support/markup.htests/unit/feature/hover_tests.cpptests/unit/support/markup_tests.cpptests/unit/support/structed_text_tests.cpp
💤 Files with no reviewable changes (1)
- tests/unit/support/structed_text_tests.cpp
Comment on lines
+646
to
+811
| auto present(const HoverInfo& hi) -> std::string { | ||
| Markup output; | ||
|
|
||
| Paragraph& header = output.add_heading(3); | ||
| if(hi.kind != SymbolKind::Invalid) | ||
| header.append_text(symbol_name(hi.kind).str()) | ||
| .append_text(hi.name, Paragraph::Kind::InlineCode); | ||
| else | ||
| header.append_text(hi.name, Paragraph::Kind::InlineCode); | ||
|
|
||
| output.add_ruler(); | ||
|
|
||
| if(hi.return_type) { | ||
| std::string ret_str = hi.return_type->type; | ||
| if(hi.return_type->aka) | ||
| ret_str += " (aka " + *hi.return_type->aka + ")"; | ||
| output.add_paragraph() | ||
| .append_text("\xe2\x86\x92") | ||
| .append_text(ret_str, Paragraph::Kind::InlineCode); | ||
| } | ||
|
|
||
| if(hi.parameters && !hi.parameters->empty()) { | ||
| output.add_paragraph().append_text("Parameters:"); | ||
| auto& list = output.add_bullet_list(); | ||
| for(auto& param: *hi.parameters) | ||
| list.add_item().add_paragraph().append_text(format_param(param), | ||
| Paragraph::Kind::InlineCode); | ||
| } | ||
|
|
||
| if(hi.type && !hi.return_type && !hi.parameters) { | ||
| std::string type_str = hi.type->type; | ||
| if(hi.type->aka) | ||
| type_str += " (aka " + *hi.type->aka + ")"; | ||
| output.add_paragraph().append_text("Type:").append_text(type_str, | ||
| Paragraph::Kind::InlineCode); | ||
| } | ||
|
|
||
| if(hi.value) | ||
| output.add_paragraph().append_text("Value =").append_text(*hi.value, | ||
| Paragraph::Kind::InlineCode); | ||
|
|
||
| if(hi.offset) | ||
| output.add_paragraph().append_text("Offset: " + format_offset(*hi.offset)); | ||
|
|
||
| if(hi.size) { | ||
| std::string size_text = "Size: " + format_size(*hi.size); | ||
| if(hi.padding && *hi.padding != 0) | ||
| size_text += std::format(" (+{} padding)", format_size(*hi.padding)); | ||
| if(hi.align) | ||
| size_text += ", alignment " + format_size(*hi.align); | ||
| output.add_paragraph().append_text(size_text); | ||
| } | ||
|
|
||
| if(!hi.documentation.empty()) { | ||
| output.add_ruler(); | ||
| auto [doxygen, rest] = strip_doxygen_info(hi.documentation); | ||
|
|
||
| std::string doc; | ||
|
|
||
| for(auto& [tag, contents]: doxygen.get_block_command_comments()) { | ||
| if(tag == "brief") { | ||
| for(auto& c: contents) { | ||
| auto text = llvm::StringRef(c.content).trim(); | ||
| if(!text.empty()) { | ||
| doc += text; | ||
| doc += "\n\n"; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| auto trimmed = llvm::StringRef(rest).trim(); | ||
| if(!trimmed.empty()) { | ||
| doc += trimmed; | ||
| doc += "\n\n"; | ||
| } | ||
|
|
||
| auto param_docs = doxygen.get_param_command_comments(); | ||
| if(!param_docs.empty()) { | ||
| bool rendered = false; | ||
| if(hi.parameters && !hi.parameters->empty()) { | ||
| for(auto& param: *hi.parameters) { | ||
| if(!param.name) | ||
| continue; | ||
| auto info = doxygen.find_param_info(*param.name); | ||
| if(!info) | ||
| continue; | ||
| auto content = llvm::StringRef((*info)->content).trim(); | ||
| doc += "- `"; | ||
| doc += *param.name; | ||
| doc += "`"; | ||
| if(!content.empty()) { | ||
| doc += " — "; | ||
| doc += content; | ||
| } | ||
| doc += "\n"; | ||
| rendered = true; | ||
| } | ||
| } | ||
| if(!rendered) { | ||
| for(auto& [name, info]: param_docs) { | ||
| auto content = llvm::StringRef(info->content).trim(); | ||
| doc += "- `"; | ||
| doc += name; | ||
| doc += "`"; | ||
| if(!content.empty()) { | ||
| doc += " — "; | ||
| doc += content; | ||
| } | ||
| doc += "\n"; | ||
| } | ||
| } | ||
| doc += "\n"; | ||
| } | ||
|
|
||
| if(auto ret = doxygen.get_return_info()) { | ||
| auto text = ret->trim(); | ||
| if(!text.empty()) { | ||
| doc += "**Returns:** "; | ||
| doc += text; | ||
| doc += "\n\n"; | ||
| } | ||
| } | ||
|
|
||
| for(auto& [tag, contents]: doxygen.get_block_command_comments()) { | ||
| if(tag == "brief") | ||
| continue; | ||
| std::string label = tag.str(); | ||
| if(!label.empty()) | ||
| label[0] = std::toupper(label[0]); | ||
| for(auto& c: contents) { | ||
| auto text = llvm::StringRef(c.content).trim(); | ||
| if(!text.empty()) { | ||
| doc += "**"; | ||
| doc += label; | ||
| doc += ":** "; | ||
| doc += text; | ||
| doc += "\n\n"; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| auto final_doc = llvm::StringRef(doc).trim(); | ||
| if(!final_doc.empty()) | ||
| output.add_paragraph().append_text(final_doc.str()); | ||
| } | ||
|
|
||
| if(!hi.definition.empty()) { | ||
| output.add_ruler(); | ||
|
|
||
| std::string code; | ||
| if(!hi.local_scope.empty()) { | ||
| code += "// In " + llvm::StringRef(hi.local_scope).rtrim(':').str() + '\n'; | ||
| } else if(hi.namespace_scope && !hi.namespace_scope->empty()) { | ||
| code += | ||
| "// In namespace " + llvm::StringRef(*hi.namespace_scope).rtrim(':').str() + '\n'; | ||
| } | ||
|
|
||
| if(!hi.access_specifier.empty()) | ||
| code += hi.access_specifier + ": "; | ||
|
|
||
| code += hi.definition; | ||
| output.add_code_block(std::move(code), "cpp"); | ||
| } | ||
|
|
||
| return output.as_markdown(); |
There was a problem hiding this comment.
Honor HoverOptions in the new documentation renderer.
This path always strips Doxygen commands and always emits markdown-style formatting, but HoverOptions::enable_doxygen_parsing and HoverOptions::parse_comment_as_markdown never reach present(). Callers can no longer opt out of either behavior, even though those flags are part of the public API in src/feature/feature.h.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/feature/hover.cpp` around lines 646 - 811, The present(const HoverInfo&
hi) renderer currently always strips Doxygen and emits markdown; update it to
honor HoverOptions by adding a HoverOptions const& options parameter to present
(and update its callers) and gate the Doxygen/markdown behavior: only call
strip_doxygen_info/use markdown formatting when options.enable_doxygen_parsing
and options.parse_comment_as_markdown are true respectively (leave current
behavior when those flags are true, and if false, append raw/rest text without
Doxygen parsing or markdown decoration). Ensure you reference the HoverInfo
users of strip_doxygen_info and the parts that build doc/param/return blocks so
they run conditionally based on the new HoverOptions flags.
Comment on lines
+189
to
191
| void Markup::append(Markup& other) { | ||
| std::move(other.blocks.begin(), other.blocks.end(), std::back_inserter(blocks)); | ||
| } |
There was a problem hiding this comment.
append() leaves the source document in a crashing state.
After this move, other.blocks still has the same length but now contains null unique_ptrs. A later other.as_markdown() or second append() will walk those entries in render_blocks() and dereference null.
Suggested fix
void Markup::append(Markup& other) {
std::move(other.blocks.begin(), other.blocks.end(), std::back_inserter(blocks));
+ other.blocks.clear();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/support/markup.cpp` around lines 189 - 191, Markup::append currently
moves pointers out of other.blocks but leaves other.blocks the same length
filled with null unique_ptrs, causing later calls (other.as_markdown,
render_blocks, or a second append) to dereference nulls; fix by moving with
proper move-iterators (e.g., std::make_move_iterator) into blocks and then clear
or swap out other.blocks so its size becomes zero (e.g., other.blocks.clear() or
other.blocks = {};), ensuring other.blocks contains no null entries after
Markup::append.
…tructedText) - Parse documentation through strip_doxygen_info() to render \param, \return, \brief and other doxygen commands as structured markdown instead of raw text - Fix markdown block separation: add newline between blocks so paragraphs, bullet lists, and code blocks no longer merge on the same line - Fix CodeBlock closing fence not on its own line when code lacks trailing newline - Fix Heading::clone() slicing to Paragraph (lost heading level on copy) - Fix BulletList multi-line items missing continuation indentation - Fix double backtick in hover heading (name wrapped manually + InlineCode) - Rename StructedText → Markup, fix Strikethough → Strikethrough typo - Add DoxygenInfo::get_param_command_comments() for param iteration - Rewrite Markup tests from 3 assertion-less smoke tests to 27 proper tests - Add 6 doxygen-specific hover tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ode rendering Single newline caused setext heading interpretation (text\n--- → H2) and paragraph merging in CommonMark/markdown-it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use the project's .clang-format style to format the code snippets shown in hover popups, so they match the user's formatting preferences instead of Clang's raw pretty-printer output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/hover-and-markup-rewrite
branch
from
May 4, 2026 12:37
df33bed to
f397e95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/feature/formatting.cpp (1)
74-80: 💤 Low valueConsider logging on silent failure paths for consistency.
document_formatemitsLOG_WARNwhen formatting fails;format_codesilently falls back to the original code on both the!styleand!resultpaths. When hover formatting silently produces unformatted output, there's no signal to diagnose why.🪵 Proposed addition
if(!style) + LOG_WARN("Failed to get format style for {}: {}", file, llvm::toString(style.takeError())); return code.str(); auto replacements = clang::format::reformat(*style, code, {tooling::Range(0, code.size())}); auto result = tooling::applyAllReplacements(code, replacements); if(!result) + LOG_WARN("Failed to apply format replacements for {}: {}", file, llvm::toString(result.takeError())); return code.str();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feature/formatting.cpp` around lines 74 - 80, The function format_code currently returns the original code silently when no style is available or when applyAllReplacements fails; add LOG_WARN calls on those failure paths to mirror document_format's behavior: in format_code (the branch checking if(!style)) log a warning like "format_code: no clang style available, returning original code" including any relevant context, and in the branch after tooling::applyAllReplacements (if(!result)) log a warning like "format_code: failed to apply replacements, returning original code" including result.error() or any diagnostic info available so hover formatting failures are visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/support/markup.h`:
- Around line 68-71: The add_item() method returns a Markup& but items is a
std::vector<Markup>, which can relocate elements on growth and produce dangling
references; change the container type of items from std::vector<Markup> to
std::deque<Markup> (include <deque>) so push_back does not invalidate
references, leaving add_item()'s signature unchanged and ensuring returned
references remain valid even after subsequent insertions.
---
Nitpick comments:
In `@src/feature/formatting.cpp`:
- Around line 74-80: The function format_code currently returns the original
code silently when no style is available or when applyAllReplacements fails; add
LOG_WARN calls on those failure paths to mirror document_format's behavior: in
format_code (the branch checking if(!style)) log a warning like "format_code: no
clang style available, returning original code" including any relevant context,
and in the branch after tooling::applyAllReplacements (if(!result)) log a
warning like "format_code: failed to apply replacements, returning original
code" including result.error() or any diagnostic info available so hover
formatting failures are visible.
🪄 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: 5eefc1a4-fda2-4218-aa4f-825e9899f282
📒 Files selected for processing (11)
src/feature/feature.hsrc/feature/formatting.cppsrc/feature/hover.cppsrc/support/doxygen.cppsrc/support/doxygen.hsrc/support/markup.cppsrc/support/markup.htests/unit/feature/formatting_tests.cpptests/unit/feature/hover_tests.cpptests/unit/support/markup_tests.cpptests/unit/support/structed_text_tests.cpp
💤 Files with no reviewable changes (1)
- tests/unit/support/structed_text_tests.cpp
✅ Files skipped from review due to trivial changes (2)
- src/support/doxygen.cpp
- src/feature/hover.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/support/doxygen.h
- src/support/markup.cpp
Comment on lines
+68
to
+71
| Markup& add_item(); | ||
|
|
||
| private: | ||
| std::vector<StructedText> items; | ||
| std::vector<Markup> items; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/support/markup.hRepository: clice-io/clice
Length of output: 3038
🏁 Script executed:
find . -name "*.cpp" -o -name "*.cc" | xargs rg -l "add_item" | head -5Repository: clice-io/clice
Length of output: 145
🏁 Script executed:
cat -n src/support/markup.cppRepository: clice-io/clice
Length of output: 7644
🏁 Script executed:
cat -n tests/unit/support/markup_tests.cpp | head -100Repository: clice-io/clice
Length of output: 3265
🏁 Script executed:
cat -n tests/unit/support/markup_tests.cpp | grep -A 20 "BulletList\|add_item"Repository: clice-io/clice
Length of output: 3600
🏁 Script executed:
rg "add_item\(\)" --context 2 -A 2Repository: clice-io/clice
Length of output: 2282
add_item() exposes dangling references after later insertions.
add_item() returns a Markup&, but items is a std::vector<Markup>. Any later growth can relocate the stored elements, so a caller that keeps the first returned reference and then adds another item can end up writing through a dangling reference. While current usage patterns avoid this, the API design invites the misuse.
Replace std::vector with std::deque, which does not invalidate references to existing elements on push_back:
Suggested fix
+#include <deque>
`#include` <memory>
`#include` <string>
`#include` <vector>
@@
private:
- std::vector<Markup> items;
+ std::deque<Markup> items;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/support/markup.h` around lines 68 - 71, The add_item() method returns a
Markup& but items is a std::vector<Markup>, which can relocate elements on
growth and produce dangling references; change the container type of items from
std::vector<Markup> to std::deque<Markup> (include <deque>) so push_back does
not invalidate references, leaving add_item()'s signature unchanged and ensuring
returned references remain valid even after subsequent insertions.
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
StructedText→Markupwith comprehensive fixes to the markdown renderer (block separation, bullet list indentation, heading clone, code block fences)TODO (differences from clangd)
auto/decltypeto show deduced typeTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests