Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @aurora0x27, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors and enhances the hover functionality within the codebase. The changes aim to make the hover feature more robust, flexible, and capable of providing more comprehensive information by introducing optional return types, configurable options, and improved handling of various Abstract Syntax Tree (AST) node types, particularly TypeLocs.
Highlights
- Hover Function Signature Update: The primary
hoverfunction ininclude/Feature/Hover.handsrc/Feature/Hover.cpphas been updated to returnstd::optional<Hover>instead ofHover, indicating that hover information might not always be available. It also now accepts aconfig::HoverOptionsparameter for more configurable behavior. - Improved Hover Information Retrieval: Internal helper functions like
getHoverItems,getDocument,getQualifier, andgetSourceCodehave been refactored (e.g.,getHoverItemsrenamed toget_hover_itemsand now acceptsconfig::HoverOptions). New overloads forhoverandget_hover_itemshave been added to specifically handleclang::TypeLocnodes, expanding the types of symbols for which hover information can be generated. - Integration of Hover Options: The
src/Server/Feature.cppfile now initializes and passesconfig::HoverOptionsto thefeature::hovercall, enabling features like Doxygen parsing, Markdown comment parsing, and showing 'aka' information for hover results. - Enhanced AST Traversal and Logging: The
hoverimplementation insrc/Feature/Hover.cppnow includes more robust AST traversal logic, usingsrc_loc_in_main_fileandspelled_tokens_touchfor better token identification. Debugging logs (clice::log::warn,clice::log::info) have been added to aid in development and understanding of the hover logic.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the hover feature, introducing std::optional for the return type and config::HoverOptions for configurability. The changes are a good step forward, but there are a few issues to address. Most critically, there's a potential crash in src/Server/Feature.cpp due to dereferencing an optional without checking if it has a value. I've also pointed out some leftover debugging code and unused parameters that should be cleaned up before merging. Overall, good progress on this feature.
src/Server/Feature.cpp
Outdated
| auto hover = feature::hover(*ast, offset, opt); | ||
| // TODO: Join comment with ast info, build structed text | ||
| proto::Hover result; | ||
| result.contents.kind = "markdown"; | ||
| result.contents.value = std::format("{}: {}", hover.kind.name(), hover.name); | ||
| result.contents.value = std::format("{}: {}", hover->kind.name(), hover->name); | ||
| return json::serialize(result); |
There was a problem hiding this comment.
The function feature::hover now returns a std::optional<Hover>. However, the returned optional is dereferenced on line 70 (hover->kind.name()) without first checking if it contains a value. If feature::hover returns std::nullopt, this will cause a crash. You should check if hover has a value before accessing it.
auto hover = feature::hover(*ast, offset, opt);
if (!hover) {
return json::Value(nullptr);
}
// TODO: Join comment with ast info, build structed text
proto::Hover result;
result.contents.kind = "markdown";
result.contents.value = std::format("{}: {}", hover->kind.name(), hover->name);
return json::serialize(result);
include/Feature/Hover.h
Outdated
| // /// Generate the hover information for the given declaration(for test). | ||
| // Hover hover(CompilationUnit& unit, const clang::NamedDecl* decl, const config::HoverOptions& opt); |
There was a problem hiding this comment.
It's generally better to remove commented-out code rather than leaving it in the codebase. If this function is no longer needed, please remove it to improve code clarity. If it's intended for future use, a // TODO comment explaining the plan would be more informative.
src/Feature/Hover.cpp
Outdated
| std::string getDocument(CompilationUnit& unit, | ||
| const clang::NamedDecl* decl, | ||
| config::HoverOptions opt) { |
There was a problem hiding this comment.
The opt parameter of type config::HoverOptions has been added to this function but is not being used within the function body. This also applies to getQualifier and getSourceCode. Unused parameters can be misleading. Please either use this parameter to configure the function's behavior or remove it if it's not needed.
src/Feature/Hover.cpp
Outdated
| for(auto& tk: tokens_under_cursor) { | ||
| clice::log::info("Hit token '{}'", tk.str()); | ||
| } |
There was a problem hiding this comment.
This loop appears to be for debugging purposes, as it logs every token under the cursor. This kind of verbose logging should be removed from the final code to avoid cluttering the logs in production.
src/Feature/Hover.cpp
Outdated
|
|
||
| clice::log::warn("Not selected"); | ||
|
|
||
| node->data.dump(llvm::errs(), unit.context()); |
There was a problem hiding this comment.
This line dumps AST node data directly to stderr. This is useful for debugging but should not be part of the final implementation. Please remove this line.
aurora0x27
force-pushed
the
main
branch
2 times, most recently
from
3b966a5 to
635cf85
Compare
|
Any update? |
Handling typeloc and template params. |
aurora0x27
force-pushed
the
main
branch
2 times, most recently
from
78d65fe to
c518757
Compare
|
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:
📝 WalkthroughWalkthroughRefactors hover to return optional results with configurable HoverOptions; expands HoverItem kinds/values; adds type-printing, advanced type deduction (auto/decltype), macro/include/directive handling, highlight ranges, richer hover rendering, and many new unit tests. Public hover API now returns std::optional and accepts HoverOptions. Changes
Sequence DiagramsequenceDiagram
participant User
participant Server as Server (on_hover)
participant HoverAPI as Hover Feature
participant AST as Compiler/AST/SelectionTree
participant Deducer as Type Deducer
participant Renderer as Display Renderer
User->>Server: hover request (file, offset)
Server->>HoverAPI: hover(unit, offset, HoverOptions)
HoverAPI->>AST: map offset → Node / SourceLocation
alt Macro / Include / Directive
HoverAPI->>AST: resolve macro/include & expansions
HoverAPI->>HoverAPI: build directive/macro hover items
else AST node
HoverAPI->>HoverAPI: route by node kind (Decl / TypeLoc / Expr)
alt auto / decltype / template-deduced
HoverAPI->>Deducer: deduce concrete QualType
Deducer->>AST: query instantiation/context
Deducer->>HoverAPI: return deduced type
end
HoverAPI->>HoverAPI: collect HoverItems (name, type, size, offset, docs)
end
HoverAPI->>Server: return optional<Hover> (with hl_range)
Server->>Renderer: hover.display(HoverOptions)
Server->>User: formatted markdown + optional range
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/Feature/Hover.h (1)
1-6: Header uses STL types without including their headers. Add required includes (and forward-declare CompilationUnit).Public headers must be self-sufficient. Please include the STL headers you use and forward-declare CompilationUnit (or include its header).
Apply this diff near the top:
#pragma once -#include "AST/SymbolKind.h" -#include "AST/SourceCode.h" -#include "Index/Shared.h" +#include "AST/SymbolKind.h" +#include "Index/Shared.h" +#include <cstdint> +#include <optional> +#include <string> +#include <vector> + +namespace clice { +class CompilationUnit; +} // namespace cliceOptional: drop the unused include to reduce dependencies as shown above.
src/Feature/Hover.cpp (1)
15-35: Use wide types and consistent units for hover metrics (bytes vs bits).
- getFieldOffset returns bits; your comment suggests “Offset” (commonly bytes). Convert to bytes.
- Char/align sizes use CharUnits->getQuantity (signed 64). Avoid truncation to uint32_t.
- auto addItem = [&items](HoverItem::HoverKind kind, uint32_t value) { - items.emplace_back(kind, llvm::Twine(value).str()); - }; + auto addItem = [&items](HoverItem::HoverKind kind, uint64_t value) { + items.emplace_back(kind, llvm::Twine(value).str()); + }; @@ - addItem(HoverItem::FieldIndex, FD->getFieldIndex()); - addItem(HoverItem::Offset, Ctx.getFieldOffset(FD)); - addItem(HoverItem::Size, Ctx.getTypeSizeInChars(FD->getType()).getQuantity()); - addItem(HoverItem::Align, Ctx.getTypeAlignInChars(FD->getType()).getQuantity()); + addItem(HoverItem::FieldIndex, static_cast<uint64_t>(FD->getFieldIndex())); + // Convert bits -> bytes for user-facing offset. + addItem(HoverItem::Offset, + static_cast<uint64_t>(Ctx.getFieldOffset(FD)) / + static_cast<uint64_t>(Ctx.getCharWidth())); + addItem(HoverItem::Size, + static_cast<uint64_t>(Ctx.getTypeSizeInChars(FD->getType()).getQuantity())); + addItem(HoverItem::Align, + static_cast<uint64_t>(Ctx.getTypeAlignInChars(FD->getType()).getQuantity()));Also clarify units in HoverItem comments if keeping bits anywhere.
♻️ Duplicate comments (4)
include/Feature/Hover.h (1)
68-70: Remove commented-out API or add TODO with rationale.Dead commented code increases noise; delete it or replace with a clear TODO and target owner.
-// /// Generate the hover information for the given declaration(for test). -// Hover hover(CompilationUnit& unit, const clang::NamedDecl* decl, const config::HoverOptions& -// opt);src/Feature/Hover.cpp (2)
115-118: Reduce noisy token logging to debug or remove in release builds.
This was previously flagged; switch to debug or guard with a compile-time flag.- for(auto& tk: tokens_under_cursor) { - clice::logging::info("Hit token '{}'", tk.str()); - } + for(auto& tk: tokens_under_cursor) { + clice::logging::debug("Hit token '{}'", tk.str()); + }
138-138: Remove raw AST dump to stderr or guard it behind debug.
Keeps logs clean in production.- node->data.dump(llvm::errs(), unit.context()); + // node->data.dump(llvm::errs(), unit.context()); // debug onlysrc/Server/Feature.cpp (1)
65-71: Check optional before use and capture by value to avoid lifetime hazards.
- The optional is dereferenced without a has_value() check. Return null when no hover exists.
- Avoid capturing stack locals by reference in async work; capture by value (both opt and ast pointer are small).
- co_return co_await async::submit([kind = this->kind, offset, &ast, &opt] { - auto hover = feature::hover(*ast, offset, opt); - // TODO: Join comment with ast info, build structed text - proto::Hover result; - result.contents.kind = "markdown"; - result.contents.value = std::format("{}: {}", hover->kind.name(), hover->name); - return json::serialize(result); - }); + co_return co_await async::submit([kind = this->kind, offset, ast = ast, opt = opt] { + auto hover = feature::hover(*ast, offset, opt); + if(!hover) { + return json::Value(nullptr); + } + // TODO: Join comment with AST info; build structured text + proto::Hover result; + result.contents.kind = "markdown"; + result.contents.value = std::format("{}: {}", hover->kind.name(), hover->name); + return json::serialize(result); + });
🧹 Nitpick comments (4)
include/Feature/Hover.h (1)
72-75: Public API looks good; consider documenting nullopt cases and option semantics.Briefly document when nullopt is returned and how HoverOptions affects output for LSP clients.
src/Server/Feature.cpp (1)
60-64: Initialize HoverOptions from configuration once, not per request.The in-function NOTE is correct. Plumb user/server settings and store defaults on Server; read here without reconstructing each call.
src/Feature/Hover.cpp (2)
46-76: Unify HoverOptions passing and silence unused warnings.These helpers accept HoverOptions by value, unlike others by const-ref; also currently unused. Take by const-ref and mark maybe_unused to avoid warnings until options are applied.
-std::string getDocument(CompilationUnit& unit, - const clang::NamedDecl* decl, - config::HoverOptions opt) { +std::string getDocument(CompilationUnit& unit, + const clang::NamedDecl* decl, + [[maybe_unused]] const config::HoverOptions& opt) { @@ -std::string getQualifier(CompilationUnit& unit, - const clang::NamedDecl* decl, - config::HoverOptions opt) { +std::string getQualifier(CompilationUnit& unit, + const clang::NamedDecl* decl, + [[maybe_unused]] const config::HoverOptions& opt) { @@ -std::string getSourceCode(CompilationUnit& unit, - const clang::NamedDecl* decl, - config::HoverOptions opt) { +std::string getSourceCode(CompilationUnit& unit, + const clang::NamedDecl* decl, + [[maybe_unused]] const config::HoverOptions& opt) {Later, wire options into formatting/parsing behaviors.
98-104: TypeLoc path returns nullopt; consider minimal fallback content.Even a simple spelling of the type under cursor would improve UX until full support is added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/Feature/Hover.h(1 hunks)src/Feature/Hover.cpp(3 hunks)src/Server/Feature.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
include/Feature/Hover.h (2)
src/Feature/Hover.cpp (6)
hover(85-96)hover(85-87)hover(98-104)hover(98-100)hover(106-154)hover(106-108)include/Compiler/CompilationUnit.h (1)
CompilationUnit(15-204)
src/Feature/Hover.cpp (2)
src/AST/Selection.cpp (12)
unit(718-727)unit(718-721)range(407-437)range(407-407)offset(592-606)offset(592-592)offset(1131-1136)offset(1131-1131)create_right(1233-1240)create_right(1233-1233)node(978-990)node(978-978)src/AST/Utility.cpp (2)
name_of(193-253)name_of(193-193)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (6)
hover(85-96)hover(85-87)hover(98-104)hover(98-100)hover(106-154)hover(106-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: build (macos-15)
- GitHub Check: build (windows-2025)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (1)
src/Feature/Hover.cpp (1)
106-114: Return early on invalid loc is good; consider validating offset before building SelectionTree.After adding the bounds check in src_loc_in_main_file, this is largely covered. Optional: also verify offset <= content size here for defense-in-depth.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/Feature/Hover.cpp (3)
140-196: Consider reducing logging verbosity in node dispatch.This function logs every node kind it encounters (lines 145, 168, 171, 177, 181, 184, 188). While useful during development, this level of logging will be very noisy in production. Consider moving to debug level or removing once the hover dispatching logic is stable.
205-207: Validate offset bounds before constructing SourceLocation.The helper constructs a
SourceLocationfor any offset without checking if the offset is within the file bounds. Out-of-range offsets can produce invalid locations and cause downstream crashes.Apply this diff to add bounds checking:
auto src_loc_in_main_file = [&sm](uint32_t off) -> std::optional<clang::SourceLocation> { + auto fid = sm.getMainFileID(); + auto size = sm.getFileIDSize(fid); + if(off >= size) { + return std::nullopt; + } - return sm.getLocForStartOfFile(sm.getMainFileID()).getLocWithOffset(off); + return sm.getLocForStartOfFile(fid).getLocWithOffset(off); };
130-138: Remove debug AST dump to stderr.Line 135 dumps the TypeLoc AST to stderr on every type hover. This is useful for development but should be removed before merging, as it clutters stderr and exposes internal AST details.
static std::optional<Hover> hover(CompilationUnit& unit, const clang::TypeLoc* typeloc, const config::HoverOptions& opt) { // TODO: Hover for type clice::logging::warn("Hit a typeloc"); - typeloc->dump(llvm::errs(), unit.context()); auto ty = typeloc->getType(); return Hover{.kind = SymbolKind::Type, .name = ty.getAsString()}; }tests/unit/Feature/Hover.cpp (1)
173-187: The test scaffolding is incomplete (same issue as DeducedType_Auto).The
offsetsvector is empty, so the loop doesn't test anything. Populate it with the position markers fromannotation.offsetsbefore merging.include/Feature/Hover.h (1)
77-79: Remove the commented-out code.Commented-out code should be removed to keep the codebase clean. If this declaration is needed in the future, it can be retrieved from version control history.
Apply this diff:
-// /// Generate the hover information for the given declaration(for test). -// Hover hover(CompilationUnit& unit, const clang::NamedDecl* decl, const config::HoverOptions& -// opt); - /// Generate the hover information for the symbol at the given offset.
🧹 Nitpick comments (2)
src/Server/Feature.cpp (1)
60-63: Consider making HoverOptions configurable.The
HoverOptionsare hardcoded here. If hover behavior should be user-configurable (via LSP settings or workspace config), consider sourcing these options from a configuration manager rather than hardcoding them in the handler.src/Feature/Hover.cpp (1)
29-56: Consider reducing debug logging verbosity.Lines 30, 53-55 emit warnings on every field hover. These logs will be very noisy in production when users hover over struct members. Consider using
debuglevel or removing them once the feature stabilizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/Feature/Hover.h(3 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)tests/unit/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
tests/unit/Feature/Hover.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (12)
get_item_content(250-257)get_item_content(250-250)display(259-272)display(259-259)hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
src/Feature/Hover.cpp (1)
src/AST/Selection.cpp (11)
unit(718-727)unit(718-721)kind(1185-1190)kind(1185-1185)os(1154-1154)range(407-437)range(407-407)node(978-990)node(978-978)create_right(1233-1240)create_right(1233-1233)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (macos-15)
- GitHub Check: build (windows-2025)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (2)
src/Server/Feature.cpp (1)
68-78: Hover result handling looks correct.The code properly checks the optional hover result and the optional display result before using them, with appropriate fallback messages. This addresses the previous review concern about null dereference.
src/Feature/Hover.cpp (1)
259-272: The display formatting is basic but functional.The current implementation provides a straightforward text-based representation. This works for a WIP feature. If richer formatting is desired (collapsible sections, syntax highlighting hints, formatted tables), that can be deferred to a follow-up.
| std::vector<unsigned> offsets; | ||
| tester.compile(); | ||
| expect(tester.unit.has_value()); | ||
| for(auto offset: offsets) { | ||
| auto HI = clice::feature::hover(*tester.unit, offset, {}); | ||
| if(HI.has_value()) { | ||
| auto msg = HI->display({}); | ||
| // std::println("{}", *msg); | ||
| } else { | ||
| // std::println("No hover info"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test scaffolding is incomplete.
The offsets vector is declared but never populated, so the loop at lines 101-109 iterates over an empty collection and doesn't exercise any hover functionality. Ensure you populate offsets from the annotation.offsets map (e.g., by extracting pos_0, pos_1, etc.) before merging this PR.
Apply this pattern to populate the offsets:
auto annotation = AnnotatedSource::from(code);
tester.add_main("main.cpp", annotation.content);
- std::vector<unsigned> offsets;
+ std::vector<unsigned> offsets = {
+ annotation.offsets["pos_0"],
+ annotation.offsets["pos_1"],
+ // ... add remaining positions
+ };
tester.compile();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/unit/Feature/Hover.cpp around lines 98 to 109, the offsets vector is
declared but never populated so the hover loop never runs; populate offsets from
the test annotation's offsets map before calling tester.compile(), e.g. extract
entries like "pos_0", "pos_1", ... (or iterate all annotation.offsets keys) and
push their integer values into the offsets vector so the for-loop exercises
hover at those positions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
include/Feature/Hover.h (1)
77-79: Remove or document the commented-out function.As noted in a past review, commented-out code should be removed to improve clarity. If this function is intended for future use, replace the comment with a
// TODOexplaining the plan.src/Feature/Hover.cpp (6)
18-63: Theoptparameter is unused in this function.As noted in a past review, the
config::HoverOptions& optparameter is declared but not used withinget_hover_items. Either use it to configure behavior (e.g., controlling which items to include) or remove it if it's not needed yet.
83-90: Theoptparameter is unused.As noted in a past review, the
config::HoverOptions optparameter is not used inget_qualifier. Consider removing it or using it to configure output formatting.
71-81: The function retrieves the comment but returns an empty string.As flagged in a past review, line 79 retrieves
raw_stringbut line 80 returns an empty string, so documentation is never shown in hover results.Return the raw string until parsing is fully implemented:
auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string;
130-138: Remove the debug dump to stderr.As flagged in a past review, line 135 dumps the AST node to
stderr. This debug output should be removed or conditionally compiled for production.- clice::logging::warn("Hit a typeloc"); - typeloc->dump(llvm::errs(), unit.context()); auto ty = typeloc->getType();
140-196: Excessive debug logging should be removed or reduced.As noted in a past review, this function contains extensive
clice::logging::warncalls that will clutter production logs. Either remove them, replace with debug-level logging, or conditionally compile them.
205-207: Validate offset bounds to prevent crashes.As flagged in a past review, the lambda at lines 205-207 doesn't validate that
offis within the main file's size, which can lead to invalidSourceLocationobjects and downstream crashes.Add bounds checking:
auto src_loc_in_main_file = [&sm](uint32_t off) -> std::optional<clang::SourceLocation> { - return sm.getLocForStartOfFile(sm.getMainFileID()).getLocWithOffset(off); + auto fid = sm.getMainFileID(); + auto buf = sm.getBufferData(fid); + if(off > buf.size()) { + return std::nullopt; + } + return sm.getLocForStartOfFile(fid).getLocWithOffset(off); };tests/unit/Feature/Hover.cpp (3)
98-109: The offsets vector is never populated.As flagged in the previous review, the
offsetsvector is declared but never populated fromannotation.offsets, so the loop at lines 101-109 never runs and the test doesn't exercise hover functionality.Populate the offsets vector before the loop:
auto annotation = AnnotatedSource::from(code); tester.add_main("main.cpp", annotation.content); - std::vector<unsigned> offsets; + std::vector<unsigned> offsets; + for (const auto& [key, offset] : annotation.offsets) { + offsets.push_back(offset); + } tester.compile();
158-158: Fix the malformed token.Line 158 contains
decltyppos_13)ype(auto)which is syntactically incorrect. This should bedeclty$(pos_13)pe(auto)to match the annotation pattern used elsewhere.Apply this fix:
- decltyppos_13)ype(auto) r2 = get_ca(); // const int + declty$(pos_13)pe(auto) r2 = get_ca(); // const int
173-187: The offsets vector is never populated.Same issue as in "DeducedType_Auto": the
offsetsvector at line 173 is declared but never populated fromannotation.offsets, so the loop at lines 177-187 doesn't exercise any hover functionality.Populate the offsets vector:
auto annotation = AnnotatedSource::from(code); tester.add_main("main.cpp", annotation.content); - std::vector<unsigned> offsets; + std::vector<unsigned> offsets; + for (const auto& [key, offset] : annotation.offsets) { + offsets.push_back(offset); + } tester.compile();
🧹 Nitpick comments (3)
src/Server/Feature.cpp (2)
60-63: Consider externalizing the hover configuration.The
HoverOptionsinstance is hardcoded inline. If these settings are reused across multiple features or need to be user-configurable, consider moving them to a server-level configuration or a dedicated config file.
68-78: Improve fallback messages for production readiness.The fallback messages "Cannot display hover info" and "Cannot get hover info" are appropriate for debugging but may confuse end users. Consider returning a more informative message or silently returning null to let the client handle the absence of hover data gracefully.
For example:
- } else { - clice::logging::warn("Cannot display hover info"); - result.contents.value = "Cannot display hover info"; - } + } else { + clice::logging::warn("Cannot display hover info"); + return json::Value(nullptr); + } } else { clice::logging::warn("Cannot get hover info"); - result.contents.value = "Cannot get hover info"; + return json::Value(nullptr); }src/Feature/Hover.cpp (1)
110-115: Theoptparameter is unused.The
config::HoverOptions optparameter is not used in this overload ofget_source_code. Consider removing it or using it to control source code formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/Feature/Hover.h(3 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)tests/unit/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
tests/unit/Feature/Hover.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (12)
get_item_content(250-257)get_item_content(250-250)display(259-272)display(259-259)hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
src/Feature/Hover.cpp (2)
src/AST/Selection.cpp (15)
unit(718-727)unit(718-721)kind(1185-1190)kind(1185-1185)os(1154-1154)range(407-437)range(407-407)node(978-990)node(978-978)offset(592-606)offset(592-592)offset(1131-1136)offset(1131-1131)create_right(1233-1240)create_right(1233-1233)src/AST/Utility.cpp (3)
os(375-375)name_of(219-279)name_of(219-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-15)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (6)
src/Server/Feature.cpp (1)
68-78: LGTM: Optional checking correctly implemented.The code now properly checks both
hoverandhover->display(opt)before accessing their values, preventing the null-pointer dereference flagged in the past review.tests/unit/Feature/Hover.cpp (1)
35-55: LGTM: Inclusion test is complete.The "Inclusion" test properly extracts offsets from annotations and exercises the hover functionality for include directives.
include/Feature/Hover.h (3)
38-39: LGTM: Enum rename improves clarity.Renaming
EnumValuetoValuebroadens the semantic scope to cover variables, constants, and enum items, aligning with the updated comment.
68-74: LGTM: New methods support config-driven hover display.The added
hl_range,get_item_content, anddisplaymembers enable flexible hover content retrieval and formatting, integrating well with the updated hover API.
82-84: LGTM: Public API updated to return optional and accept config.The hover function signature now returns
std::optional<Hover>and acceptsconfig::HoverOptions, enabling guarded code paths and config-driven behavior.src/Feature/Hover.cpp (1)
250-272: LGTM: Hover display methods are well-structured.The
get_item_contentanddisplayimplementations provide clean accessors for hover data, enabling flexible presentation in the server layer.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/unit/Feature/Hover.cpp (3)
98-109: Test scaffolding still incomplete - offsets vector never populated.The
offsetsvector is declared at line 98 but never populated with values fromannotation.offsets. The loop at lines 101-109 iterates over an empty collection and doesn't exercise any hover functionality. This is the same issue flagged in the previous review.Populate the offsets vector before the loop:
auto annotation = AnnotatedSource::from(code); tester.add_main("main.cpp", annotation.content); - std::vector<unsigned> offsets; + std::vector<unsigned> offsets; + for(auto& [key, value] : annotation.offsets) { + offsets.push_back(value); + } tester.compile();
158-158: Fix malformed decltype syntax.Line 158 contains
decltyppos_13)ype(auto)which is malformed. This appears to be a typo mixing position marker annotation with thedecltypekeyword.Fix the syntax:
- decltyppos_13)ype(auto) r2 = get_ca(); // const int + decl$(pos_13)type(auto) r2 = get_ca(); // const int
173-187: Test scaffolding incomplete - offsets vector never populated.The
offsetsvector at line 173 is declared but never populated with values fromannotation.offsets. The loop at lines 177-187 iterates over an empty collection and doesn't test any hover functionality.Populate the offsets vector:
auto annotation = AnnotatedSource::from(code); tester.add_main("main.cpp", annotation.content); - std::vector<unsigned> offsets; + std::vector<unsigned> offsets; + for(auto& [key, value] : annotation.offsets) { + offsets.push_back(value); + } tester.compile();include/Feature/Hover.h (1)
77-79: Remove commented-out code.Rather than leaving the old function declaration commented out, please remove these lines entirely. If the signature needs to be referenced later, it will be available in version control history.
-// /// Generate the hover information for the given declaration(for test). -// Hover hover(CompilationUnit& unit, const clang::NamedDecl* decl, const config::HoverOptions& -// opt); -src/Feature/Hover.cpp (3)
71-81: Critical: Documentation is retrieved but never returned.Line 79 successfully retrieves
raw_stringfrom the raw comment, but line 80 immediately returns an empty string without using it. This means hover will never display documentation even when comments are present. This critical issue from the previous review remains unresolved.Return the raw string:
auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string;
130-138: Remove debug output before merging.Line 135 dumps AST node data directly to
stderrviatypeloc->dump(), and line 134 logs a warning. This debug instrumentation was flagged in the previous review and should be removed from the final implementation.static std::optional<Hover> hover(CompilationUnit& unit, const clang::TypeLoc* typeloc, const config::HoverOptions& opt) { // TODO: Hover for type - clice::logging::warn("Hit a typeloc"); - typeloc->dump(llvm::errs(), unit.context()); auto ty = typeloc->getType(); return Hover{.kind = SymbolKind::Type, .name = ty.getAsString()}; }
205-207: Critical: Validate offset bounds before constructing SourceLocation.The lambda
src_loc_in_main_fileat lines 205-207 constructs aSourceLocationfrom an arbitrary offset without validating that the offset is within the file's bounds. Out-of-range offsets can produce invalid locations and cause downstream crashes. This critical issue from the previous review remains unresolved.Add bounds checking:
auto src_loc_in_main_file = [&sm](uint32_t off) -> std::optional<clang::SourceLocation> { + auto fid = sm.getMainFileID(); + auto buf = sm.getBufferData(fid, /*Invalid=*/nullptr); + if(off > buf.size()) { + return std::nullopt; + } - return sm.getLocForStartOfFile(sm.getMainFileID()).getLocWithOffset(off); + return sm.getLocForStartOfFile(fid).getLocWithOffset(off); };Verify this pattern is correct for Clang's SourceManager API:
#!/bin/bash # Search for similar offset validation patterns in the codebase rg -nP -C3 'getBufferData|getBuffer.*size|FileIDSize' --type=cpp
🧹 Nitpick comments (3)
src/Feature/Hover.cpp (3)
18-63: Consider reducing logging verbosity in production.The function has several
logging::warncalls (lines 30, 53-55, 58) that appear to be for development/debugging. While acceptable during active development, consider moving these to debug-level logging or removing them before the feature is finalized.
140-196: Consider reducing logging verbosity.This function contains extensive
logging::warncalls at lines 145, 168, 171, 177, 181, 184, and 188. While useful during development, this level of logging may be excessive for production. Consider reducing to debug-level or removing most of these before finalization.
215-248: Include hover logic looks good; consider reducing logging.The logic for detecting and handling hover on include directives (lines 215-238) is well-structured. However, logging statements at lines 222 and 244 could be reduced or removed in the final version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/Feature/Hover.h(3 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)tests/unit/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unit/Feature/Hover.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (12)
get_item_content(250-257)get_item_content(250-250)display(259-272)display(259-259)hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
src/Feature/Hover.cpp (1)
src/AST/Selection.cpp (15)
unit(718-727)unit(718-721)kind(1185-1190)kind(1185-1185)os(1154-1154)range(407-437)range(407-407)node(978-990)node(978-978)offset(592-606)offset(592-592)offset(1131-1136)offset(1131-1131)create_right(1233-1240)create_right(1233-1233)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: windows (windows-2025)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
🔇 Additional comments (5)
src/Server/Feature.cpp (1)
60-80: Previous critical issue resolved - proper optional handling implemented.The implementation now correctly checks both
hoverandhover->display(opt)before accessing their values, with appropriate fallback messages. This addresses the critical null-dereference concern from the previous review.The hardcoded
HoverOptionsat lines 61-63 could be extracted to a configuration class or constant for better maintainability, but this is acceptable for now.tests/unit/Feature/Hover.cpp (1)
35-55: Test exercises include hover functionality.The test successfully exercises hover on include directives at two different positions. While it doesn't yet assert specific outcomes, this is reasonable for work-in-progress testing to validate the basic code path.
include/Feature/Hover.h (2)
6-6: API enhancements look good.The additions are well-structured:
- Protocol/Basic.h include supports the new
hl_rangefield- Renamed enum value from
EnumValuetoValuewith clearer documentation- New
get_item_contentanddisplaymethods provide a clean API for hover content retrieval and formattingAlso applies to: 38-39, 68-74
82-84: Updated signature aligns with refactoring goals.The new signature returning
std::optional<Hover>and acceptingconfig::HoverOptionsproperly supports cases where hover information is unavailable and enables configurable hover behavior.src/Feature/Hover.cpp (1)
250-272: Helper methods are well-implemented.Both
get_item_contentanddisplaymethods provide clean, functional implementations:
get_item_contentefficiently searches for items by kinddisplayproduces well-structured, readable output with proper formatting
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
include/Feature/Hover.h (1)
77-79: Remove or document the commented code.This was previously flagged: commented-out code should either be removed for clarity or accompanied by a TODO comment explaining its future purpose.
tests/unit/Feature/Hover.cpp (2)
98-109: The offsets vector remains unpopulated.This issue was previously flagged: the
offsetsvector is declared but never populated, so the hover loop never runs. You need to extract positions fromannotation.offsets(e.g.,pos_0,pos_1, etc.) before the loop.
158-158: Fix the malformed token.This typo was previously flagged:
decltyppos_13)ypeshould be corrected to properdecltypesyntax with an appropriate position marker.src/Feature/Hover.cpp (2)
71-81: The get_document bug remains unfixed.This critical issue was previously flagged: line 79 retrieves
raw_stringfrom the comment, but line 80 immediately returns an empty string, so documentation is never shown in hover. At minimum, returnraw_stringinstead of"".
130-138: Remove the stderr dump.Line 135 dumps AST data directly to
stderr, which was previously flagged for removal. This debug artifact should not be in production code.
🧹 Nitpick comments (2)
src/Feature/Hover.cpp (2)
18-63: Consider using debug log level for routine operations.Lines 30, 53-55, and 58 use
warnlevel for normal hover item construction. Since these are routine operations (not warnings), consider usingdebuglevel to avoid cluttering production logs.
140-196: Consider reducing logging verbosity.This function contains numerous
warn-level logging statements (lines 145, 168, 171, 177, 181, 184, 188) that trace normal control flow. For production code, either remove these logs or change them todebuglevel to avoid cluttering logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/Feature/Hover.h(3 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)tests/unit/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
tests/unit/Feature/Hover.cpp (1)
src/Feature/Hover.cpp (8)
hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (12)
get_item_content(250-257)get_item_content(250-250)display(259-272)display(259-259)hover(117-128)hover(117-119)hover(130-138)hover(130-132)hover(140-196)hover(140-142)hover(200-248)hover(200-202)
src/Feature/Hover.cpp (3)
src/AST/Selection.cpp (15)
unit(718-727)unit(718-721)kind(1185-1190)kind(1185-1185)os(1154-1154)range(407-437)range(407-407)node(978-990)node(978-978)offset(592-606)offset(592-592)offset(1131-1136)offset(1131-1131)create_right(1233-1240)create_right(1233-1233)include/AST/SourceCode.h (2)
range(70-94)clang(24-38)src/Compiler/Directive.cpp (16)
loc(25-35)loc(25-28)loc(37-47)loc(37-40)loc(145-166)loc(145-146)loc(168-172)loc(168-170)loc(174-179)loc(174-177)loc(181-185)loc(181-183)loc(188-192)loc(188-190)loc(195-200)loc(195-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: build (macos-15)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: linux (ubuntu-24.04, release)
🔇 Additional comments (2)
tests/unit/Feature/Hover.cpp (1)
35-55: The Inclusion test structure looks good.The test correctly extracts offsets from annotations and exercises the hover feature for include directives. The unused
HIvariable is acceptable for work-in-progress testing.src/Server/Feature.cpp (1)
60-80: LGTM! Proper null checks and error handling.The hover implementation now correctly checks both the hover result and display result before dereferencing, with appropriate fallback messages. This addresses the previous review concern about missing null checks.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
include/Feature/Hover.h (1)
77-79: Remove the stale commented-out declaration.
Line 78 still carries the commentedhoverdeclaration. Either delete it or replace it with a documented TODO so it doesn’t linger as dead code.src/Feature/Hover.cpp (3)
146-148: Drop thetypeloc->dumpdebug print.
Line 147 dumps AST data tostderrduring normal hover handling; this floods logs in production. Remove the dump (or gate it behind a debug knob).- clice::logging::warn("Hit a typeloc"); - typeloc->dump(llvm::errs(), unit.context()); + clice::logging::debug("Hit a TypeLoc");
262-264: Null-checkgetFileEntryForID.
Line 263 dereferencessm.getFileEntryForID(inclusion.fid)without verifying it’s non-null; missing entries will crash the server.- auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + }
83-93: Returnstd::optionalfromget_document.
Lines 85-92 always return an empty string, soHover::documentbecomes engaged even when no comment exists, producing empty “Document” blocks. change the helper to surfacestd::nulloptunless a non-empty comment is available.-static std::string get_document(CompilationUnit& unit, - const clang::NamedDecl* decl, - config::HoverOptions opt) { +static std::optional<std::string> get_document(CompilationUnit& unit, + const clang::NamedDecl* decl, + config::HoverOptions opt) { clang::ASTContext& Ctx = unit.context(); const clang::RawComment* comment = Ctx.getRawCommentForAnyRedecl(decl); - if(!comment) { - return ""; - } - auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + if(!comment) { + return std::nullopt; + } + auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); + if(raw_string.empty()) { + return std::nullopt; + } + // TODO: respect opt.parse_comment_as_markdown when formatting + return raw_string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/Feature/Hover.h(3 hunks)include/Protocol/Feature/Hover.h(1 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)src/Server/Lifecycle.cpp(1 hunks)tests/unit/Feature/Hover.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/Feature/Hover.cpp
- src/Server/Feature.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (12)
get_item_content(329-336)get_item_content(329-329)display(338-356)display(338-338)hover(129-140)hover(129-131)hover(142-151)hover(142-144)hover(153-220)hover(153-155)hover(224-327)hover(224-226)
src/Feature/Hover.cpp (3)
include/AST/SourceCode.h (2)
range(70-94)clang(24-38)src/AST/Selection.cpp (18)
range(407-437)range(407-407)begin(565-589)begin(565-565)unit(718-727)unit(718-721)kind(1185-1190)kind(1185-1185)node(978-990)node(978-978)offset(592-606)offset(592-592)offset(1131-1136)offset(1131-1131)fid(513-562)fid(513-514)create_right(1233-1240)create_right(1233-1233)src/AST/Utility.cpp (2)
name_of(219-279)name_of(219-219)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/Server/Lifecycle.cpp (1)
38-39: The compile_commands.json loading remains disabled.This change was flagged in previous reviews as critical. Commenting out the compilation database loading removes essential compiler context (include paths, flags, defines) needed for accurate hover information and other LSP features. Since this PR is marked "In process," ensure this line is restored before merging.
src/Feature/Hover.cpp (3)
36-38: The add_item lambda has a potential compilation issue.The lambda calls
items.emplace_back(kind, val), butHoverItemis an aggregate with no constructor taking(HoverKind, std::string). This was flagged in a previous review.Apply this fix:
auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { - items.emplace_back(kind, val); + items.emplace_back(HoverItem{kind, std::move(val)}); };
83-93: The get_document function returns an empty string instead of the comment text.Line 91 retrieves the raw comment but line 92 immediately returns an empty string, so documentation is never displayed. This was flagged in a previous review.
Apply this fix:
static std::string get_document(CompilationUnit& unit, const clang::NamedDecl* decl, config::HoverOptions opt) { clang::ASTContext& Ctx = unit.context(); const clang::RawComment* comment = Ctx.getRawCommentForAnyRedecl(decl); if(!comment) { return ""; } auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string; }
253-267: Missing null check before dereferencing FileEntry.Line 262 calls
sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName()without checking ifgetFileEntryForIDreturns nullptr. This was flagged in a previous review.Apply this defensive check:
auto raw_name = get_source_code(unit, inclusion.filename_range); auto file_name = llvm::StringRef{raw_name}.trim("<>\""); Hover hi; hi.kind = SymbolKind::Directive; hi.name = file_name; - auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + } // TODO: Provides symbol return hi;
🧹 Nitpick comments (4)
src/Server/Feature.cpp (1)
60-63: Consider loading HoverOptions from server configuration.The options are currently hardcoded inline. Moving these to the server's configuration structure would improve maintainability and allow users to customize hover behavior without code changes.
tests/unit/Feature/Hover.cpp (1)
156-156: The offsets vector is declared but never used.The vector is initialized but the test directly extracts offsets from
annotation.offsetsin the loop below (lines 161-162). This variable can be removed.auto annotation = AnnotatedSource::from(code); tester.add_main("main.cpp", annotation.content); - std::vector<unsigned> offsets; tester.compile();include/Feature/Hover.h (1)
77-79: Commenting out the test helper function is acceptable.Since this function is for testing and may be re-enabled later, commenting it out is reasonable for a work-in-progress PR. Consider removing it entirely if it won't be used.
src/Feature/Hover.cpp (1)
42-42: Consider reducing debug logging verbosity.The
logging::warncalls throughout the implementation (lines 42, 65-67, 70, and many more below) are useful during development but should be reduced or removed before merging to avoid log clutter in production. Consider usinglogging::debugor removing them.Also applies to: 65-67, 70-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/Feature/Hover.h(3 hunks)include/Protocol/Feature/Hover.h(1 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)src/Server/Lifecycle.cpp(1 hunks)tests/unit/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (8)
hover(129-140)hover(129-131)hover(142-151)hover(142-144)hover(153-220)hover(153-155)hover(224-327)hover(224-226)
include/Feature/Hover.h (2)
src/Feature/Hover.cpp (12)
get_item_content(329-336)get_item_content(329-329)display(338-356)display(338-338)hover(129-140)hover(129-131)hover(142-151)hover(142-144)hover(153-220)hover(153-155)hover(224-327)hover(224-226)include/Compiler/CompilationUnit.h (1)
CompilationUnit(15-204)
tests/unit/Feature/Hover.cpp (1)
src/Feature/Hover.cpp (8)
hover(129-140)hover(129-131)hover(142-151)hover(142-144)hover(153-220)hover(153-155)hover(224-327)hover(224-226)
src/Feature/Hover.cpp (1)
src/AST/Selection.cpp (11)
range(401-431)range(401-401)unit(712-721)unit(712-715)kind(1179-1184)kind(1179-1179)os(1148-1148)node(972-984)node(972-972)create_right(1227-1234)create_right(1227-1227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: build (macos-15)
- GitHub Check: macos (macos-15, release)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025)
- GitHub Check: windows (windows-2025)
- GitHub Check: linux (ubuntu-24.04, release)
🔇 Additional comments (18)
include/Protocol/Feature/Hover.h (1)
20-20: LGTM!The addition of
std::optional<Range>to support hover range visualization is well-documented and aligns with the broader hover feature enhancements.src/Server/Feature.cpp (1)
68-82: Good defensive handling of optional hover results.The code properly checks for
std::nulloptbefore accessing hover data and provides clear fallback messages when hover information is unavailable or cannot be displayed. This addresses the past review concern about dereferencing without checking.tests/unit/Feature/Hover.cpp (3)
35-82: LGTM!The Inclusion tests properly exercise hover functionality for include directives with both PCH and non-PCH compilation paths, correctly extracting offsets from annotations and calling the hover API.
115-170: The DeducedType_Auto test looks good.The test properly compiles annotated code and exercises hover at each annotated position with appropriate display handling.
172-245: The DeducedType_Decltype test looks good.The test properly compiles annotated code and exercises hover at each annotated position with appropriate display handling.
include/Feature/Hover.h (4)
6-6: LGTM!Adding the Protocol/Basic.h include is necessary for the proto::Range type used in hl_range.
38-39: LGTM!Renaming
EnumValuetoValuewith the updated comment better reflects the broader semantics for variables, constants, and enum items.
60-74: LGTM!The structural changes are well-designed:
- Making
documentoptional is appropriate since not all symbols have documentation.- Adding
hl_rangesupports range-based highlighting.- The new helper methods
get_item_content()anddisplay()provide clean interfaces for content extraction and rendering.
82-84: LGTM!The updated signature returning
std::optional<Hover>and acceptingconfig::HoverOptionsproperly reflects that hover may not always be available and allows for configurable behavior.src/Feature/Hover.cpp (9)
20-28: LGTM!The
to_proto_rangehelper cleanly converts Clang's SourceRange to the protocol's Range format using spelling locations.
41-72: Good implementation of hover items for fields and variables.The code properly extracts type information, offsets, alignment, size, and bitfield data for field declarations, with appropriate checks for dependent types and unions.
95-127: LGTM!The
get_qualifierandget_source_codehelper functions properly extract qualified names and source text using Clang's APIs.
129-140: LGTM!The hover function for NamedDecl cleanly aggregates all hover information using the helper functions.
153-220: The node-based hover dispatch is well-structured.The function properly dispatches to type-specific hover handlers based on AST node kind, with appropriate handling for auto/decltype types and fallbacks for unhandled node types.
229-236: LGTM!The
src_loc_in_main_filelambda now includes proper bounds checking, which addresses a previous review concern about validating offsets against file size.
270-313: Good implementation of macro hover support.The code properly extracts macro names, definitions, and expansions with appropriate range information. The macro expansion text extraction is a nice feature.
315-327: LGTM!The fallback to SelectionTree-based hover with proper range attachment is well-implemented.
329-356: LGTM!The
get_item_contentanddisplaymember functions provide clean interfaces for extracting and rendering hover information. The markdown-like formatting is readable and informative.
| auto annotation = AnnotatedSource::from(code); | ||
| tester.add_main("main.cpp", annotation.content); | ||
| for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) {} | ||
| }; |
There was a problem hiding this comment.
The Macros test loop body is empty.
The test sets up annotated code with macro positions but the loop at line 112 iterates over annotation.offsets.size() without actually calling the hover API or making any assertions. This test doesn't verify any behavior.
Populate the loop body to exercise hover functionality:
auto annotation = AnnotatedSource::from(code);
tester.add_main("main.cpp", annotation.content);
- for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) {}
+ tester.compile();
+ expect(tester.unit.has_value());
+ for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) {
+ unsigned offset = annotation.offsets[std::format("pos_{}", idx)];
+ auto HI = clice::feature::hover(*tester.unit, offset, {});
+ if(HI.has_value()) {
+ auto msg = HI->display({});
+ std::println("```\n{}```\n", *msg);
+ } else {
+ std::println("No hover info for pos_{}", idx);
+ }
+ }📝 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.
| auto annotation = AnnotatedSource::from(code); | |
| tester.add_main("main.cpp", annotation.content); | |
| for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) {} | |
| }; | |
| auto annotation = AnnotatedSource::from(code); | |
| tester.add_main("main.cpp", annotation.content); | |
| tester.compile(); | |
| expect(tester.unit.has_value()); | |
| for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) { | |
| unsigned offset = annotation.offsets[std::format("pos_{}", idx)]; | |
| auto HI = clice::feature::hover(*tester.unit, offset, {}); | |
| if(HI.has_value()) { | |
| auto msg = HI->display({}); | |
| std::println(" |
🤖 Prompt for AI Agents
In tests/unit/Feature/Hover.cpp around lines 110 to 113, the for-loop over
annotation.offsets is empty so the test never exercises hover behavior; update
the loop to iterate each annotated offset, convert each offset to a
file/position, call the hover API (via the existing tester helper), and assert
or check the returned hover response (e.g., expect non-empty hover or match
against annotation.expected content), otherwise print/log "No hover info for
pos_{idx}" to make failures visible; ensure any required file/URI/position
construction uses the same helpers as above and add appropriate assertions so
the test validates hover results.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
src/Feature/Hover.cpp (5)
37-39: Fix aggregate construction inadd_itemlambda.This issue was previously flagged:
HoverItemhas no constructor taking(HoverKind, std::string). The lambda must construct the aggregate explicitly.Apply this diff:
auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { - items.emplace_back(kind, val); + items.emplace_back(HoverItem{kind, std::move(val)}); };
84-94: Fix the documentation retrieval logic.This critical issue was previously flagged: Line 92 retrieves
raw_stringbut Line 93 immediately returns an empty string, so hover will never display documentation.Apply this diff to return the retrieved documentation:
static std::string get_document(CompilationUnit& unit, const clang::NamedDecl* decl, config::HoverOptions opt) { clang::ASTContext& Ctx = unit.context(); const clang::RawComment* comment = Ctx.getRawCommentForAnyRedecl(decl); if(!comment) { return ""; } auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string; }
86-86: Address unusedoptparameters.This issue was previously flagged: The
optparameter of typeconfig::HoverOptionsis not used inget_document,get_qualifier, orget_source_code. Unused parameters can be misleading.Either implement the configuration options (e.g., parse comments as markdown based on
opt.parse_comment_as_markdown) or remove the parameters if they're not yet needed.Also applies to: 98-98, 327-327
349-350: Remove debug dump to stderr.This issue was previously flagged: Line 350 dumps AST data directly to stderr. This is useful for debugging but should not be part of the final implementation.
Apply this diff:
static std::optional<Hover> hover(CompilationUnit& unit, const clang::TypeLoc* typeloc, const config::HoverOptions& opt) { // TODO: Hover for type LOGGING_WARN("Hit a typeloc"); - typeloc->dump(); auto ty = typeloc->getType();
467-468: Add null check for FileEntry before dereferencing.This critical issue was previously flagged: Line 467 calls
sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName()without checking ifgetFileEntryForIDreturns nullptr.Apply this defensive check:
Hover hi; hi.kind = SymbolKind::Directive; hi.name = file_name; - auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + } // TODO: Provides symbol return hi;
🧹 Nitpick comments (1)
src/Feature/Hover.cpp (1)
476-477: Remove or downgrade verbose logging.This logging may be useful for development but should be removed or converted to debug-level logging for production.
auto tokens_under_cursor = unit.spelled_tokens_touch(*loc); if(tokens_under_cursor.empty()) { - LOGGING_WARN("Cannot detect tokens"); return std::nullopt; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Feature/Hover.cpp (2)
src/AST/Selection.cpp (17)
range(401-431)range(401-401)begin(559-583)begin(559-559)unit(712-721)unit(712-715)kind(1179-1184)kind(1179-1179)os(1148-1148)node(972-984)node(972-972)offset(586-600)offset(586-586)offset(1125-1130)offset(1125-1125)create_right(1227-1234)create_right(1227-1227)src/Compiler/CompilationUnit.cpp (2)
resolver(317-320)resolver(317-317)
🔇 Additional comments (5)
src/Feature/Hover.cpp (5)
486-488: Verify the need to remove the last character from macro name.Line 487 calls
pop_back()on the macro name without explanation. This seems unusual and could indicate a workaround for an extraction issue.Verify that this is intentional. If
get_source_codeconsistently includes an unwanted trailing character, consider fixing the root cause in the extraction function rather than working around it here. Add a comment explaining why this is necessary if it's intentional.
118-155: Template helper functions look good.The
getContainedAutoParamTypeand template instantiation helpers are well-structured and handle the necessary type unwrapping and template specialization logic correctly.
165-303: DeducedTypeVisitor implementation is well-structured.The visitor pattern correctly handles various auto/decltype type deduction scenarios including function returns, declarators, and template parameters. The logic for unwrapping types and finding instantiations appears sound.
Note the FIXME at line 191 regarding resolver usage for undeduced auto types in variable declarations.
434-441: Offset validation is correctly implemented.The bounds checking added here addresses a previously flagged critical issue. The function now validates that the offset is within the file buffer before constructing a SourceLocation, preventing potential crashes from out-of-range offsets.
543-561: Display method provides good structured output.The
displaymethod correctly formats hover information with items, documentation, and source code. The TODO comment appropriately indicates future markdown generation capability.
src/Feature/Hover.cpp
Outdated
| LOGGING_WARN("Hit a `NestedNameSpecifierLoc`"); | ||
| } else if(is_in_range(QualType, TypeLoc)) { | ||
| // Typeloc | ||
| LOGGING_WARN("Hit a `TypeLoc`"); | ||
| if(auto typeloc = node->get<clang::TypeLoc>()) { | ||
| return hover(unit, typeloc, opt); | ||
| } | ||
| } else if(is_in_range(Decl, Stmt)) { | ||
| // Decl | ||
| LOGGING_WARN("Hit a `Decl`"); | ||
| if(auto decl = node->get<clang::NamedDecl>()) { | ||
| return hover(unit, decl, opt); | ||
| } else { | ||
| LOGGING_WARN("Not intersted"); | ||
| } | ||
| } else if(is_in_range(Attr, ObjCProtocolLoc)) { | ||
| LOGGING_WARN("Hit an `Attr`"); | ||
| // TODO: Attr | ||
| } else { | ||
| // Not interested | ||
| LOGGING_WARN("Not interested"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reduce verbose logging in node routing logic.
These logging statements appear to be for debugging the node kind routing logic. They should be removed or converted to debug-level logging to avoid cluttering production logs.
Consider removing these logging statements:
if(is(NestedNameSpecifierLoc)) {
- LOGGING_WARN("Hit a `NestedNameSpecifierLoc`");
} else if(is_in_range(QualType, TypeLoc)) {
// Typeloc
- LOGGING_WARN("Hit a `TypeLoc`");
if(auto typeloc = node->get<clang::TypeLoc>()) {
return hover(unit, typeloc, opt);
}
} else if(is_in_range(Decl, Stmt)) {
// Decl
- LOGGING_WARN("Hit a `Decl`");
if(auto decl = node->get<clang::NamedDecl>()) {
return hover(unit, decl, opt);
} else {
- LOGGING_WARN("Not intersted");
}
} else if(is_in_range(Attr, ObjCProtocolLoc)) {
- LOGGING_WARN("Hit an `Attr`");
// TODO: Attr
} else {
// Not interested
- LOGGING_WARN("Not interested");
}🤖 Prompt for AI Agents
In src/Feature/Hover.cpp around lines 397 to 417 the code emits multiple
LOGGING_WARN messages used only for routing/debugging; change these to a
debug-level logger (e.g., LOGGING_DEBUG) or remove them entirely to avoid
verbose production logs, keeping only meaningful warnings/errors; update the
messages consistently (or drop the "Not interested"/"Not intersted" ones) so
routing logic no longer floods warning logs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
src/Feature/Hover.cpp (10)
43-43: Remove verbose debug logging.This development logging statement clutters production logs and was flagged in previous reviews.
Apply this diff:
if(auto fd = llvm::dyn_cast<clang::FieldDecl>(decl)) { - LOGGING_WARN("Got a field decl"); const auto record = fd->getParent();
66-68: Remove verbose debug logging.This was flagged in previous reviews. Routine field processing should not emit warnings to production logs.
Apply this diff:
if(fd->isBitField()) { add_item(HoverItem::BitWidth, llvm::Twine(fd->getBitWidthValue()).str()); - LOGGING_WARN("Got bit field, name: {}, bitwidth: {}", - fd->getName(), - fd->getBitWidthValue()); }
71-71: Remove verbose debug logging.This development logging statement was flagged in previous reviews and should be removed.
Apply this diff:
} else if(auto vd = llvm::dyn_cast<clang::VarDecl>(decl)) { - LOGGING_WARN("Got a var decl"); add_item(HoverItem::Type, vd->getType().getAsString());
392-413: Reduce verbose logging in node routing logic.These logging statements were flagged in a previous review. They clutter production logs and should be removed or converted to debug-level logging.
Consider removing or downgrading these logging statements:
if(is(NestedNameSpecifierLoc)) { - LOGGING_WARN("Hit a `NestedNameSpecifierLoc`"); } else if(is_in_range(QualType, TypeLoc)) { - LOGGING_WARN("Hit a `TypeLoc`"); if(auto typeloc = node->get<clang::TypeLoc>()) { return hover(unit, typeloc->getType(), opt); } } else if(is_in_range(Decl, Stmt)) { - LOGGING_WARN("Hit a `Decl`"); if(auto decl = node->get<clang::NamedDecl>()) { return hover(unit, decl, opt); } else { - LOGGING_WARN("Not intersted"); } } else if(is_in_range(Attr, ObjCProtocolLoc)) { - LOGGING_WARN("Hit an `Attr`"); // TODO: Attr } else { - LOGGING_WARN("Not interested"); }
471-471: Remove verbose debug logging.This was flagged in previous reviews and clutters production logs.
Apply this diff:
auto tokens_under_cursor = unit.spelled_tokens_touch(*loc); if(tokens_under_cursor.empty()) { - LOGGING_WARN("Cannot detect tokens"); return std::nullopt; }
516-524: Remove verbose debug logging.These logging statements were flagged in a previous review and should be removed to keep production logs clean.
auto tree = SelectionTree::create_right(unit, {offset, offset}); if(auto node = tree.common_ancestor()) { - LOGGING_WARN("Got node: {}", node->kind()); if(auto info = hover(unit, node, opt)) { info->hl_range = to_proto_range(sm, hl_range); return info; } return std::nullopt; } else { - LOGGING_WARN("Not an ast node"); }
37-38: Fixadd_itemconstruction.This was flagged in a previous review:
HoverItemhas no(HoverKind, std::string)constructor, soitems.emplace_back(kind, val)will not compile. Build the aggregate explicitly.Apply this diff:
- auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { - items.emplace_back(kind, val); + auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { + items.emplace_back(HoverItem{kind, std::move(val)}); };
78-82: TypeLoc hover items implementation is empty.This was flagged in a previous review. Add a TODO comment to track future work if this is planned.
Add a TODO comment:
static std::vector<HoverItem> get_hover_items(CompilationUnit& unit, const clang::TypeLoc* typeloc, const config::HoverOptions& opt) { + // TODO: Implement hover items for TypeLoc (e.g., size, alignment) return {}; }
84-94: Fix the logic bug in get_document.This was flagged in a previous review. Line 92 retrieves the raw comment but line 93 returns an empty string, so documentation is never shown.
Apply this diff:
auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string; }
462-463: Check for null FileEntry before dereferencing.This was marked as addressed in previous commits, but the code still dereferences
getFileEntryForIDwithout checking for nullptr, which can crash.Apply this defensive check:
hi.kind = SymbolKind::Directive; hi.name = file_name; - auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + } // TODO: Provides symbol
🧹 Nitpick comments (1)
src/Feature/Hover.cpp (1)
47-48: Remove commented-out code.Commented code marked for removal creates maintenance debt. Either delete it or move it behind a debug flag if needed for development.
Apply this diff:
add_item(HoverItem::Type, fd->getType().getAsString()); - /// Remove in release mode - /// add_item(HoverItem::FieldIndex, llvm::Twine(fd->getFieldIndex()).str()); if(!record->isDependentType()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Feature/Hover.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Feature/Hover.cpp (2)
src/AST/Selection.cpp (17)
range(401-431)range(401-401)begin(559-583)begin(559-559)unit(712-721)unit(712-715)kind(1179-1184)kind(1179-1179)os(1148-1148)node(972-984)node(972-972)offset(586-600)offset(586-586)offset(1125-1130)offset(1125-1125)create_right(1227-1234)create_right(1227-1227)src/Compiler/CompilationUnit.cpp (2)
resolver(317-320)resolver(317-317)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, release)
🔇 Additional comments (1)
src/Feature/Hover.cpp (1)
480-482: Review comment remains valid—request manual verification of thepop_back()behavior.The investigation confirms this is an unusual pattern. The
token.range().toCharRange().getAsRange()conversion sequence appears only once in the codebase (line 480), andpop_back()is only applied here when extracting macro token names. Line 457 uses a different strategy—.trim()with known delimiters—suggesting these are intentional but unexplained differences.The boundary calculation through
clang::Lexer::getLocForEndOfToken()combined withCharSourceRange::getCharRange()andgetAsRange()could plausibly result in an off-by-one character. However, without test execution or Clang API validation, the correctness cannot be definitively determined. The "TODO: Found macro" comment suggests this code may still be under development.Please verify: (1) that macro names are extracted correctly with the
pop_back()call, (2) whether test coverage exists for this code path, and (3) whether similar token range conversions elsewhere in the codebase should also apply this trimming.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
include/Feature/Hover.h (1)
78-80: Remove commented-out code.Per the past review comment, commented-out code should be removed. If this function is needed for testing, consider keeping it with appropriate visibility or adding a TODO explaining when it will be restored.
src/Feature/Hover.cpp (5)
155-160: Add TODO comment for unimplemented TypeLoc hover items.Per past review, this empty implementation should have a TODO to track planned work.
618-626: Consider reducing verbose debug logging.Per past reviews,
LOG_WARNstatements like "Got node", "Not an ast node" are development debugging aids. Consider removing or converting to debug-level logging before merging.
106-108: Fixadd_itemaggregate construction.Per past review,
HoverItemhas no(HoverKind, std::string)constructor. Build the aggregate explicitly and move the value.auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { - items.emplace_back(kind, val); + items.emplace_back(HoverItem{kind, std::move(val)}); };
162-173: Fixget_documentto return the raw comment.Per past review, this function retrieves the comment but returns an empty string. Return
raw_stringor add a TODO explaining why it's intentionally empty.auto raw_string = comment->getRawText(Ctx.getSourceManager()).str(); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string;
566-567: Check for null FileEntry before dereferencing.Per past review,
getFileEntryForIDcan return nullptr. Add a null check.- auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + }
🧹 Nitpick comments (4)
include/Feature/Hover.h (1)
25-25: Fix typo in comment."drieved" should be "derived".
- // NOTE: Use a set of drieved classes? + // NOTE: Use a set of derived classes?tests/unit/Feature/HoverTests.cpp (1)
166-168: Prefer assertions over print statements in tests.Using
std::printlnfor output doesn't validate correctness. Consider addingASSERT_TRUE/EXPECT_*assertions to verify the hover results match expectations, or at minimum use proper test framework logging.src/Feature/Hover.cpp (2)
148-150: Unused variablety.The variable
tyis assigned but never used.} else if(auto vd = llvm::dyn_cast<clang::VarDecl>(decl)) { - auto ty = vd->getType(); + // TODO: Add size/align info for VarDecl if non-dependent }
460-461: Remove unused variablewanted_node.
wanted_nodeis declared but never used.- auto wanted_node = node; auto Kind = node->data.getNodeKind();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/Feature/Hover.h(4 hunks)include/Protocol/Feature/Hover.h(1 hunks)src/Feature/Hover.cpp(1 hunks)src/Server/Feature.cpp(1 hunks)tests/unit/Feature/HoverTests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- include/Protocol/Feature/Hover.h
- src/Server/Feature.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (10)
display(640-658)display(640-640)hover(411-422)hover(411-413)hover(424-430)hover(424-426)hover(432-524)hover(432-434)hover(528-629)hover(528-530)
tests/unit/Feature/HoverTests.cpp (1)
src/Feature/Hover.cpp (8)
hover(411-422)hover(411-413)hover(424-430)hover(424-426)hover(432-524)hover(432-434)hover(528-629)hover(528-530)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
🔇 Additional comments (4)
include/Feature/Hover.h (3)
39-40: LGTM!The rename from
EnumValuetoValuewith an updated comment clarifying its broader use for variables (initialization/constant) and enum items is a sensible generalization.
68-75: LGTM!The additions of
hl_rangefor highlight range tracking,get_item_contentfor item lookup, anddisplayfor rendering are well-structured. Thedisplaymethod acceptingconfig::HoverOptionsby value is appropriate for a small options struct.
83-85: LGTM!The updated API signature returning
std::optional<Hover>with an additionalconfig::HoverOptionsparameter is a good design choice—it clearly signals when hover info may not be available and allows configurable behavior.src/Feature/Hover.cpp (1)
631-658: LGTM!The
get_item_contentanddisplayimplementations are straightforward and functional. The display output format is clear and includes proper markdown formatting for documentation and source code sections.
src/Feature/Hover.cpp
Outdated
| if(is(NestedNameSpecifierLoc)) { | ||
| if(auto ns_specifier_loc = node->get<NestedNameSpecifierLoc>()) { | ||
| LOG_WARN("Hit a `NestedNameSpecifierLoc`"); | ||
| if(auto ns_specifier = ns_specifier_loc->getNestedNameSpecifier()) { | ||
| auto ns = ns_specifier->getAsNamespace(); | ||
| assert(ns); | ||
| std::string name; | ||
| if(!ns->isAnonymousNamespace()) { | ||
| name = ns->getNameAsString(); | ||
| } else { | ||
| name = "Anonymous"; | ||
| } | ||
| return Hover{.kind = SymbolKind::Namespace, .name = name}; | ||
| } else { | ||
| LOG_WARN("Cannot get namespace"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle null namespace gracefully instead of asserting.
getAsNamespace() can return null for non-namespace specifiers (e.g., type specifiers like Foo::). The assert(ns) will crash in release builds. Handle this case gracefully.
if(is(NestedNameSpecifierLoc)) {
if(auto ns_specifier_loc = node->get<NestedNameSpecifierLoc>()) {
LOG_WARN("Hit a `NestedNameSpecifierLoc`");
if(auto ns_specifier = ns_specifier_loc->getNestedNameSpecifier()) {
auto ns = ns_specifier->getAsNamespace();
- assert(ns);
+ if(!ns) {
+ // Not a namespace specifier (could be a type specifier)
+ return std::nullopt;
+ }
std::string name;📝 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.
| if(is(NestedNameSpecifierLoc)) { | |
| if(auto ns_specifier_loc = node->get<NestedNameSpecifierLoc>()) { | |
| LOG_WARN("Hit a `NestedNameSpecifierLoc`"); | |
| if(auto ns_specifier = ns_specifier_loc->getNestedNameSpecifier()) { | |
| auto ns = ns_specifier->getAsNamespace(); | |
| assert(ns); | |
| std::string name; | |
| if(!ns->isAnonymousNamespace()) { | |
| name = ns->getNameAsString(); | |
| } else { | |
| name = "Anonymous"; | |
| } | |
| return Hover{.kind = SymbolKind::Namespace, .name = name}; | |
| } else { | |
| LOG_WARN("Cannot get namespace"); | |
| } | |
| } | |
| if(is(NestedNameSpecifierLoc)) { | |
| if(auto ns_specifier_loc = node->get<NestedNameSpecifierLoc>()) { | |
| LOG_WARN("Hit a `NestedNameSpecifierLoc`"); | |
| if(auto ns_specifier = ns_specifier_loc->getNestedNameSpecifier()) { | |
| auto ns = ns_specifier->getAsNamespace(); | |
| if(!ns) { | |
| // Not a namespace specifier (could be a type specifier) | |
| return std::nullopt; | |
| } | |
| std::string name; | |
| if(!ns->isAnonymousNamespace()) { | |
| name = ns->getNameAsString(); | |
| } else { | |
| name = "Anonymous"; | |
| } | |
| return Hover{.kind = SymbolKind::Namespace, .name = name}; | |
| } else { | |
| LOG_WARN("Cannot get namespace"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Feature/Hover.cpp around lines 473-489, remove the assert(ns) and instead
check if ns is null; if getAsNamespace() returns nullptr, log a warning (e.g.,
LOG_WARN("NestedNameSpecifier is not a namespace")) and return a safe Hover
value (for example with SymbolKind::Unknown and an empty name) so the code does
not crash in release builds; otherwise proceed to build the namespace name and
return the Namespace Hover as before.
| TEST_CASE(Inclusion){{tester.clear(); | ||
| constexpr auto code = R"CODE( | ||
| $(inc1)#include <iostream> | ||
|
|
||
| #include $(inc2)<assert.h> | ||
|
|
||
| int main(void) { | ||
| std::cout << "hello" << std::endl; | ||
| return 0; | ||
| } | ||
| )CODE"; | ||
| auto annotation = AnnotatedSource::from(code); | ||
| tester.add_main("main.cpp", annotation.content); | ||
| auto inc1_off = annotation.offsets["inc1"]; | ||
| auto inc2_off = annotation.offsets["inc2"]; | ||
| tester.compile_with_pch(); | ||
| ASSERT_TRUE(tester.unit.has_value()); | ||
| auto HI = clice::feature::hover(*tester.unit, inc1_off, {}); | ||
| HI = clice::feature::hover(*tester.unit, inc2_off, {}); | ||
|
|
||
| } // TEST_SUITE(Hover) | ||
|
|
||
| { | ||
| tester.clear(); | ||
| constexpr auto code = R"CODE( | ||
| #inc$(inc1)lude <assert.h> | ||
| #inc$(inc2)lude <stdio.h> | ||
|
|
||
| #define max(a, b) ((a) > (b) ? (a) : (b)) | ||
|
|
||
| int main(int argc, char **argv) { | ||
| assert(argc <= 10); | ||
| int x = max(114, 514); | ||
| printf("%d", max(argc, x)); | ||
| return 0; | ||
| } | ||
| )CODE"; | ||
| auto annotation = AnnotatedSource::from(code); | ||
| tester.add_main("main.cpp", annotation.content); | ||
| auto inc1_off = annotation.offsets["inc1"]; | ||
| auto inc2_off = annotation.offsets["inc2"]; | ||
| tester.compile(); | ||
| ASSERT_TRUE(tester.unit.has_value()); | ||
| auto HI = clice::feature::hover(*tester.unit, inc1_off, {}); | ||
| HI = clice::feature::hover(*tester.unit, inc2_off, {}); | ||
| } | ||
|
|
||
| }; // namespace |
There was a problem hiding this comment.
Critical: Fix malformed test case structure.
The brace structure here is broken. TEST_CASE(Inclusion) appears to end prematurely at line 56 with } // TEST_SUITE(Hover), but then there's a dangling code block from lines 58-81 followed by }; // namespace at line 83. This will likely cause compilation errors or unintended test behavior.
Consider restructuring:
-TEST_CASE(Inclusion){{tester.clear();
+TEST_CASE(Inclusion) {
+ tester.clear();
constexpr auto code = R"CODE(
...
-} // TEST_SUITE(Hover)
-
-{
+ // Second inclusion test case
tester.clear();
...
-}; // namespace
+}Alternatively, split this into two separate TEST_CASE blocks with descriptive names.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/unit/Feature/HoverTests.cpp around lines 36 to 83 the
TEST_CASE(Inclusion) block is prematurely closed (line ~56) leaving a dangling
standalone code block and mismatched closing comment/brace at line 83; fix by
restoring proper brace structure: either merge the dangling block into the
original TEST_CASE by removing the stray closing brace/comment so a single
TEST_CASE contains both code sections, or convert the dangling block into its
own TEST_CASE with an appropriate name and ensure each TEST_CASE has matching
opening and closing braces and that the file-level namespace/test-suite closures
remain balanced. Ensure comments reflect the corrected boundaries.
| return ca; | ||
| }; | ||
| declt$(pos_12)ype(auto) r1 = get_a(); // int& | ||
| decltyp$(pos_13)ype(auto) r2 = get_ca(); // const int |
There was a problem hiding this comment.
Typo in annotation placeholder.
decltyp$(pos_13)ype has an extra "ype" creating "decltypeype" after placeholder substitution. Should be decltype$(pos_13) or the placeholder positioned differently.
🤖 Prompt for AI Agents
In tests/unit/Feature/HoverTests.cpp around line 218, the token
"decltyp$(pos_13)ype" contains a typo where the placeholder split leaves an
extra "ype" producing "decltypeype"; replace the whole token with
"decltype$(pos_13)" so the placeholder surrounds the complete keyword (i.e.,
change decltyp$(pos_13)ype to decltype$(pos_13)) and keep the rest of the line
as-is.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
tests/unit/Feature/HoverTests.cpp (4)
36-83: Critical: Fix malformed test case structure.The brace structure is broken:
- Line 36 has double opening braces
{{- Line 56 has a premature closing comment
// TEST_SUITE(Hover)but still inside the test- Line 58 has a dangling
{block- Line 83 has
};with incorrect namespace commentThis will cause compilation errors or unintended test behavior.
Consider restructuring into proper separate test cases:
-TEST_CASE(Inclusion){{tester.clear(); +TEST_CASE(Inclusion) { + tester.clear(); constexpr auto code = R"CODE( ... -} // TEST_SUITE(Hover) - -{ +} + +TEST_CASE(Inclusion_WithMacros) { tester.clear(); ... -}; // namespace +}
113-114: Empty loop body does nothing.The loop iterates over
annotation.offsetsbut the body is empty, making the iteration pointless. Either add hover assertions inside the loop or remove it.- for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) {} + for(unsigned idx = 0; idx < annotation.offsets.size(); ++idx) { + unsigned offset = annotation.offsets[std::format("pos_{}", idx)]; + auto HI = clice::feature::hover(*tester.unit, offset, {}); + ASSERT_TRUE(HI.has_value()); + } };
218-218: Typo in annotation placeholder.
decltyp$(pos_13)ypehas an extra "ype" which will produce "decltypeype" after placeholder substitution. This should bedecltype$(pos_13)to correctly annotate the keyword.- decltyp$(pos_13)ype(auto) r2 = get_ca(); // const int + decltype$(pos_13)(auto) r2 = get_ca(); // const int
684-686: Mismatched namespace closing comments and extra semicolon.Line 684 has
};(semicolon after closing brace) and the comment// namespace clice::testingis on the wrong line. The anonymous namespace closes at line 685, andclice::testingcloses at line 686.-}; // namespace clice::testing -} // namespace +} // anonymous namespace } // namespace clice::testingsrc/Feature/Hover.cpp (6)
47-53: Logic bug: condition should check!qt.isNull().The condition
qt.isNull() && opt.show_akawill never produce the "a.k.a" output becauseqt.print(os, pp)was just called on line 46, implyingqtis valid. Ifqtwere null, desugaring would be meaningless. This should be!qt.isNull().- if(qt.isNull() && opt.show_aka) { + if(!qt.isNull() && opt.show_aka) { bool should_aka = false; auto desugared_ty = clang::desugarForDiagnostic(ctx, qt, should_aka); if(should_aka) { os << "(a.k.a " << desugared_ty.getAsString(pp) << ")"; } }
106-108: Fixadd_itemconstruction.
items.emplace_back(kind, val)will fail ifHoverItemis an aggregate without a matching two-argument constructor. Build the aggregate explicitly and move the value.auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { - items.emplace_back(kind, val); + items.emplace_back(HoverItem{kind, std::move(val)}); };
162-174: Logic bug:get_documentreturns empty string after retrieving comment.Line 171 retrieves the formatted comment into
raw_string, but line 173 immediately returns an empty string. Documentation will never be displayed.auto raw_string = comment->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics()); LOG_WARN("Got comment:\n```\n{}\n```\n", raw_string); - return ""; + return raw_string; }
476-492: Handle null namespace gracefully instead of asserting.
getAsNamespace()can return null for non-namespace specifiers (e.g., type specifiers likeFoo::). Theassert(ns)will be stripped in release builds, leading to null dereference.if(auto ns_specifier = ns_specifier_loc->getNestedNameSpecifier()) { auto ns = ns_specifier->getAsNamespace(); - assert(ns); + if(!ns) { + // Not a namespace specifier (could be a type specifier) + return std::nullopt; + } std::string name;
569-570: Check for nullFileEntrybefore dereferencing.
getFileEntryForIDcan return nullptr if the file entry is not found. This will crash.hi.kind = SymbolKind::Directive; hi.name = file_name; - auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + } // TODO: Provides symbol return hi;
587-588: Guard against empty string beforepop_back().
pop_back()on an empty string is undefined behavior.auto macro_name = get_source_code(unit, name_range); - macro_name.pop_back(); + if(!macro_name.empty()) { + macro_name.pop_back(); + }
🧹 Nitpick comments (1)
src/Feature/Hover.cpp (1)
621-628: Consider reducing verbose debug logging before merge.Multiple
LOG_WARNcalls (lines 472, 478, 490, 495, 501, 505, 508, 515, 519, 578, 621, 628) emit warnings during normal operation. These should be removed or downgraded to debug/trace level to avoid cluttering production logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Feature/Hover.cpp(1 hunks)tests/unit/Feature/HoverTests.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
src/Feature/Hover.cpp
Outdated
| if(record->isUnion()) { | ||
| add_item(HoverItem::Size, | ||
| llvm::Twine(ctx.getTypeSizeInChars(fd->getType()).getQuantity()).str()); | ||
| add_item(HoverItem::Align, | ||
| llvm::Twine(ctx.getTypeAlignInChars(fd->getType()).getQuantity()).str()); | ||
| } |
There was a problem hiding this comment.
Duplicate Size and Align items for union fields.
For union fields, lines 139-142 add Size and Align items, but for non-dependent non-union records, lines 131-135 already add Offset, Align, and Size. If a union field is in a non-dependent context, both blocks execute, resulting in duplicate Size and Align entries.
- if(record->isUnion()) {
+ if(record->isUnion() && record->isDependentType()) {
add_item(HoverItem::Size,
llvm::Twine(ctx.getTypeSizeInChars(fd->getType()).getQuantity()).str());
add_item(HoverItem::Align,
llvm::Twine(ctx.getTypeAlignInChars(fd->getType()).getQuantity()).str());
}Alternatively, restructure the logic so that union fields don't go through both branches.
📝 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.
| if(record->isUnion()) { | |
| add_item(HoverItem::Size, | |
| llvm::Twine(ctx.getTypeSizeInChars(fd->getType()).getQuantity()).str()); | |
| add_item(HoverItem::Align, | |
| llvm::Twine(ctx.getTypeAlignInChars(fd->getType()).getQuantity()).str()); | |
| } | |
| if(record->isUnion() && record->isDependentType()) { | |
| add_item(HoverItem::Size, | |
| llvm::Twine(ctx.getTypeSizeInChars(fd->getType()).getQuantity()).str()); | |
| add_item(HoverItem::Align, | |
| llvm::Twine(ctx.getTypeAlignInChars(fd->getType()).getQuantity()).str()); | |
| } |
🤖 Prompt for AI Agents
In src/Feature/Hover.cpp around lines 131 to 143, the union-specific block
(lines 139-142) and the non-dependent/non-union block (lines 131-135) can both
run, causing duplicate Size and Align entries for union fields; make the union
path mutually exclusive by restructuring the conditionals — either check
record->isUnion() first and handle/add Size/Align then skip the non-union
branch, or change the later non-union if to an else/else-if that excludes unions
(or return/continue after adding union items) so Size and Align are only added
once.
|
|
||
| // auto vs decltype(auto) | ||
| auto x1 = (a); // int | ||
| de$(pos_13)cltype(auto) x2 = (a); // int& |
There was a problem hiding this comment.
Duplicate placeholder name pos_13.
pos_13 is used on both line 218 and line 222, which will cause offset conflicts or overwrites in the annotation map.
- de$(pos_13)cltype(auto) x2 = (a); // int&
+ de$(pos_16)cltype(auto) x2 = (a); // int&📝 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.
| de$(pos_13)cltype(auto) x2 = (a); // int& | |
| de$(pos_16)cltype(auto) x2 = (a); // int& |
🤖 Prompt for AI Agents
In tests/unit/Feature/HoverTests.cpp around line 222, the placeholder name
pos_13 is duplicated (also used at line 218) causing annotation map conflicts;
rename the placeholder at line 222 to a unique identifier (e.g., pos_14) and
update any matching annotation/expectation entries that reference pos_13 for
that occurrence so each placeholder maps to a distinct offset.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (15)
include/Feature/Hover.h (1)
78-80: Remove commented-out code.Commented-out function declarations add noise. If the test hover API is needed, it should be properly implemented or tracked with a TODO/issue reference.
src/Feature/Hover.cpp (10)
619-629: Consider reducing log verbosity or using debug level.Multiple
LOG_WARNstatements throughout the hover logic are appropriate for development but should be removed or converted to debug-level logging before release to avoid cluttering production logs.
47-53: Logic bug: condition should check!qt.isNull().The condition
qt.isNull() && opt.show_akawill never be true whenqtis valid (which it is afterqt.print()on line 46). This should be!qt.isNull().🔎 Proposed fix
- if(qt.isNull() && opt.show_aka) { + if(!qt.isNull() && opt.show_aka) { bool should_aka = false; auto desugared_ty = clang::desugarForDiagnostic(ctx, qt, should_aka); if(should_aka) { os << "(a.k.a " << desugared_ty.getAsString(pp) << ")"; } }
106-108: Fixadd_itemconstruction - aggregate initialization required.
items.emplace_back(kind, val)won't compile becauseHoverItemis an aggregate with no constructor. Use aggregate initialization explicitly.🔎 Proposed fix
auto add_item = [&items](HoverItem::HoverKind kind, std::string&& val) { - items.emplace_back(kind, val); + items.emplace_back(HoverItem{kind, std::move(val)}); };
127-143: DuplicateSizeandAlignitems for union fields.When a union field is in a non-dependent context, both the non-dependent block (lines 131-136) and the union block (lines 138-143) execute, resulting in duplicate
SizeandAlignentries.🔎 Proposed fix: make branches mutually exclusive
if(auto fd = llvm::dyn_cast<clang::FieldDecl>(decl)) { const auto record = fd->getParent(); - if(!record->isDependentType()) { + if(!record->isDependentType() && !record->isUnion()) { add_item(HoverItem::Offset, llvm::Twine(ctx.getFieldOffset(fd)).str()); add_item(HoverItem::Align, llvm::Twine(ctx.getTypeAlignInChars(fd->getType()).getQuantity()).str()); add_item(HoverItem::Size, llvm::Twine(ctx.getTypeSizeInChars(fd->getType()).getQuantity()).str()); } - if(record->isUnion()) { + if(record->isUnion() && !record->isDependentType()) { add_item(HoverItem::Size,
148-150: Dead code:VarDecltype is captured but never used.The variable
tyis assigned but never utilized. Either remove the branch or add the intended hover item.
162-174:get_documentretrieves comment but returns empty string.Line 171 retrieves the formatted comment text, but line 173 returns an empty string, so hover never displays documentation.
🔎 Proposed fix
auto raw_string = comment->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics()); - LOG_WARN("Got comment:\n```\n{}\n```\n", raw_string); - return ""; + // TODO: Parse comment according to opt.parse_comment_as_markdown + return raw_string;
476-492: Handle null namespace gracefully instead of asserting.
getAsNamespace()can return null for non-namespace specifiers (e.g., type specifiers). Theassert(ns)will crash in debug builds and is stripped in release.🔎 Proposed fix
if(auto ns_specifier = ns_specifier_loc->getNestedNameSpecifier()) { auto ns = ns_specifier->getAsNamespace(); - assert(ns); + if(!ns) { + // Not a namespace specifier (could be a type specifier) + return std::nullopt; + } std::string name;
507-513: Null check beforedyn_castresult is passed tohover().
llvm::dyn_cast<NamedDecl>(vd)may return nullptr ifvdis not aNamedDecl. Passing nullptr tohover()is unsafe.🔎 Proposed fix
} else if(is(DeclRefExpr)) { LOG_WARN("Hit an `DeclRef`, Unhandled"); if(auto dr = node->get<DeclRefExpr>()) { auto vd = dr->getDecl(); - assert(vd); - return hover(unit, llvm::dyn_cast<NamedDecl>(vd), opt); + if(auto nd = llvm::dyn_cast_or_null<NamedDecl>(vd)) { + return hover(unit, nd, opt); + } }
586-588: Guard against empty string beforepop_back().Calling
pop_back()on an empty string is undefined behavior.🔎 Proposed fix
auto macro_name = get_source_code(unit, name_range); - macro_name.pop_back(); + if(!macro_name.empty()) { + macro_name.pop_back(); + }
560-574: Check for null FileEntry before dereferencing.
sm.getFileEntryForID(inclusion.fid)can return nullptr. The current code dereferences it directly without checking.🔎 Proposed fix
auto file_name = llvm::StringRef{raw_name}.trim("<>\""); Hover hi; hi.kind = SymbolKind::Directive; hi.name = file_name; - auto dir = sm.getFileEntryForID(inclusion.fid)->tryGetRealPathName(); - hi.source = dir; + if(auto file_entry = sm.getFileEntryForID(inclusion.fid)) { + hi.source = file_entry->tryGetRealPathName(); + } // TODO: Provides symbol return hi;tests/unit/Feature/HoverTests.cpp (4)
36-83: Malformed test case structure with broken braces.The
TEST_CASE(Inclusion)block has an incorrect brace structure. Line 56 prematurely ends with} // TEST_SUITE(Hover), followed by an orphaned code block (lines 58-81) and a mismatched closing at line 83.🔎 Suggested restructuring
Either merge both test blocks into a single
TEST_CASE(Inclusion)with proper braces, or split them into two distinct test cases (e.g.,TEST_CASE(Inclusion_PCH)andTEST_CASE(Inclusion_Basic)).
113-114: Empty loop body renders test ineffective.The loop iterates over offsets but performs no assertions or hover calls, making the test a no-op.
218-222: Duplicate placeholderpos_13and typo in annotation.Line 218 uses
decltyp$(pos_13)ypewhich produces "decltypeype" after substitution (extra "ype"). Line 222 reusespos_13, causing offset map conflicts.
684-686: Mismatched namespace closing comments and stray semicolon.Line 684 has
};(semicolon after brace) with incorrect comment. The comment structure is inverted relative to the actual namespace closures.
🧹 Nitpick comments (2)
src/Server/Feature.cpp (1)
61-64: Consider moving configuration to server-level settings.The inline NOTE correctly identifies that these options should be initialized from server configuration rather than hardcoded. This can be addressed in a follow-up once the hover feature stabilizes.
src/Feature/Hover.cpp (1)
643-661: LGTM ondisplay()implementation.The method correctly formats hover content with items, document, and source code. Consider whether the
std::optionalreturn type is needed since the method always returns content.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/Feature/Hover.hinclude/Protocol/Feature/Hover.hsrc/Feature/Hover.cppsrc/Server/Feature.cpptests/unit/Feature/HoverTests.cpp
🧰 Additional context used
🧬 Code graph analysis (4)
src/Server/Feature.cpp (1)
src/Feature/Hover.cpp (8)
hover(412-423)hover(412-414)hover(425-433)hover(425-427)hover(435-527)hover(435-437)hover(531-632)hover(531-533)
tests/unit/Feature/HoverTests.cpp (1)
src/Feature/Hover.cpp (8)
hover(412-423)hover(412-414)hover(425-433)hover(425-427)hover(435-527)hover(435-437)hover(531-632)hover(531-533)
src/Feature/Hover.cpp (1)
include/AST/Resolver.h (1)
clang(6-10)
include/Feature/Hover.h (1)
src/Feature/Hover.cpp (12)
get_item_content(634-641)get_item_content(634-634)display(643-661)display(643-643)hover(412-423)hover(412-414)hover(425-433)hover(425-427)hover(435-527)hover(435-437)hover(531-632)hover(531-533)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: cmake / build (macos-15, Debug)
- GitHub Check: xmake / build (macos-15, releasedbg)
- GitHub Check: xmake / build (macos-15, debug)
- GitHub Check: xmake / build (windows-2025, releasedbg)
- GitHub Check: xmake / build (ubuntu-24.04, releasedbg)
- GitHub Check: cmake / build (windows-2025, RelWithDebInfo)
- GitHub Check: xmake / build (ubuntu-24.04, debug)
- GitHub Check: cmake / build (ubuntu-24.04, Debug)
- GitHub Check: clice / publish-clice (macos-15, clice.tar.gz, clice-arm64-macos-darwin.tar.gz, clice-symbol.tar.gz, clic...
- GitHub Check: clice / publish-clice (windows-2025, clice.zip, clice-x64-windows-msvc.zip, clice-symbol.zip, clice-x64-w...
🔇 Additional comments (2)
include/Protocol/Feature/Hover.h (1)
19-20: LGTM!The optional
rangefield aligns with the LSP specification for hover responses and integrates well with thehl_rangehandling introduced in the feature layer.include/Feature/Hover.h (1)
61-75: LGTM on the Hover struct extensions.The additions of optional
document,hl_range, and theget_item_content/displaymethods are well-structured and align with the implementation insrc/Feature/Hover.cpp.
include/Feature/Hover.h
Outdated
| struct HoverItem { | ||
| // NOTE: Use a set of drieved classes? |
There was a problem hiding this comment.
Minor typo in comment.
"drieved" should be "derived".
🔎 Proposed fix
- // NOTE: Use a set of drieved classes?
+ // NOTE: Use a set of derived classes?📝 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.
| struct HoverItem { | |
| // NOTE: Use a set of drieved classes? | |
| struct HoverItem { | |
| // NOTE: Use a set of derived classes? |
🤖 Prompt for AI Agents
In include/Feature/Hover.h around lines 24-25, the comment contains a typo:
replace "drieved" with "derived" so the note reads "NOTE: Use a set of derived
classes?" and keep punctuation/spacing consistent.
src/Server/Feature.cpp
Outdated
| // NOTE: Should be initialized somewhere else | ||
| config::HoverOptions opt = {.enable_doxygen_parsing = true, | ||
| .parse_comment_as_markdown = true, | ||
| .show_aka = true}; | ||
|
|
||
| co_return co_await async::submit([kind = this->kind, offset, &ast, &opt] { | ||
| proto::Hover result; | ||
| result.contents.kind = "markdown"; | ||
| result.contents.value = std::format("{}: {}", hover.kind.name(), hover.name); | ||
| if(auto hover = feature::hover(*ast, offset, opt)) { | ||
| if(auto info = hover->display(opt)) { | ||
| result.contents.value = *info; | ||
| } else { | ||
| clice::logging::warn("Cannot display hover info"); | ||
| result.contents.value = "Cannot display hover info"; | ||
| } | ||
| if(!hover->hl_range) { | ||
| clice::logging::warn("Not available range"); | ||
| } | ||
| result.range = hover->hl_range; | ||
| } else { | ||
| clice::logging::warn("Cannot get hover info"); | ||
| result.contents.value = "Cannot get hover info"; | ||
| } | ||
| return json::serialize(result); | ||
| }); |
There was a problem hiding this comment.
Potential dangling reference: opt captured by reference in async lambda.
The local variable opt is captured by reference (&opt) in the lambda passed to async::submit. If the async task executes after on_hover returns, accessing opt would be undefined behavior.
🔎 Proposed fix: capture `opt` by value
- co_return co_await async::submit([kind = this->kind, offset, &ast, &opt] {
+ co_return co_await async::submit([kind = this->kind, offset, &ast, opt] {
proto::Hover result;
result.contents.kind = "markdown";
if(auto hover = feature::hover(*ast, offset, opt)) {📝 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.
| // NOTE: Should be initialized somewhere else | |
| config::HoverOptions opt = {.enable_doxygen_parsing = true, | |
| .parse_comment_as_markdown = true, | |
| .show_aka = true}; | |
| co_return co_await async::submit([kind = this->kind, offset, &ast, &opt] { | |
| proto::Hover result; | |
| result.contents.kind = "markdown"; | |
| result.contents.value = std::format("{}: {}", hover.kind.name(), hover.name); | |
| if(auto hover = feature::hover(*ast, offset, opt)) { | |
| if(auto info = hover->display(opt)) { | |
| result.contents.value = *info; | |
| } else { | |
| clice::logging::warn("Cannot display hover info"); | |
| result.contents.value = "Cannot display hover info"; | |
| } | |
| if(!hover->hl_range) { | |
| clice::logging::warn("Not available range"); | |
| } | |
| result.range = hover->hl_range; | |
| } else { | |
| clice::logging::warn("Cannot get hover info"); | |
| result.contents.value = "Cannot get hover info"; | |
| } | |
| return json::serialize(result); | |
| }); | |
| // NOTE: Should be initialized somewhere else | |
| config::HoverOptions opt = {.enable_doxygen_parsing = true, | |
| .parse_comment_as_markdown = true, | |
| .show_aka = true}; | |
| co_return co_await async::submit([kind = this->kind, offset, &ast, opt] { | |
| proto::Hover result; | |
| result.contents.kind = "markdown"; | |
| if(auto hover = feature::hover(*ast, offset, opt)) { | |
| if(auto info = hover->display(opt)) { | |
| result.contents.value = *info; | |
| } else { | |
| clice::logging::warn("Cannot display hover info"); | |
| result.contents.value = "Cannot display hover info"; | |
| } | |
| if(!hover->hl_range) { | |
| clice::logging::warn("Not available range"); | |
| } | |
| result.range = hover->hl_range; | |
| } else { | |
| clice::logging::warn("Cannot get hover info"); | |
| result.contents.value = "Cannot get hover info"; | |
| } | |
| return json::serialize(result); | |
| }); |
🤖 Prompt for AI Agents
In src/Server/Feature.cpp around lines 61-85, the local variable opt is
currently captured by reference in the lambda passed to async::submit which can
dangle when on_hover returns; update the lambda to capture opt by value (i.e.
make a copy in the capture list or move it into the lambda) so the async task
owns its own copy and cannot access freed stack memory, then recompile and run
tests to ensure no lifetime issues remain.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Feature/Hover.cpp`:
- Line 480: The local variable wanted_node is assigned from node in Hover.cpp
but never used; remove the unused variable assignment (the line creating
wanted_node) or, if the intent was to operate on a separate copy, replace uses
of node with wanted_node where appropriate—search for wanted_node and node in
the Hover-related functions (e.g., any hover handling logic) and either delete
the wanted_node declaration or use it consistently so the symbol is referenced.
- Around line 692-733: In Hover::display remove the unused debug variables and
paragraph: delete the local variables "content" and "os" (they are unused), and
remove the temporary debug paragraph created after the ruler (the
head_segment.add_paragraph() call that appends "Contains {} items" and the
subsequent append_newline_char()), leaving the bullet-list logic intact; ensure
you also remove the "// FIX: Delete later" comment so the function contains no
leftover debug artifacts.
In `@src/Support/Doxygen.cpp`:
- Around line 145-148: The condition currently uses bitwise OR between boolean
expressions (if(command == "b" | command == "e" | command == "c")) which
prevents short-circuiting; update the conditional to use logical OR (||) so
comparisons short-circuit correctly and maintain intended semantics for the
check on the variable command in the same block that jumps to normal_line.
In `@tests/unit/Feature/HoverTests.cpp`:
- Around line 116-184: The test DeducedType_Auto (and similar cases
DeducedType_Decltype and Comments) only prints hover results and never asserts
correctness; update the loops that call clice::feature::hover(*) storing the
result in HI to include explicit assertions (e.g., ASSERT_TRUE(HI.has_value())
before using HI->display()) for offsets where hover info is expected (the
annotated positions like pos_0..pos_7 and pos_0_i..pos_7_i), and add additional
ASSERT_EQ/ASSERT_NE on the returned display string when specific text is
expected to validate hover contents; ensure you modify both loops that iterate
over annotation.offsets and reference HI and msg so the tests fail when hover is
missing or incorrect.
🧹 Nitpick comments (1)
src/Support/Doxygen.cpp (1)
145-148:gotoacross scopes is a code smell; consider restructuring.The
goto normal_linejumps out of a nestedifblock to a label in the enclosing function scope. While valid C++, this makes control flow harder to follow. A simple restructuring (e.g., extracting the command-dispatch into a helper, or falling through with a flag) would improve readability.Also applies to: 238-241
src/Feature/Hover.cpp
Outdated
|
|
||
| #define is_in_range(LHS, RHS) (!((Kind < Flag##LHS) && is(LHS)) && (Kind < Flag##RHS)) | ||
|
|
||
| auto wanted_node = node; |
There was a problem hiding this comment.
Unused variable wanted_node.
wanted_node is assigned on line 480 but never referenced.
Proposed fix
- auto wanted_node = node;
auto Kind = node->data.getNodeKind();🤖 Prompt for AI Agents
In `@src/Feature/Hover.cpp` at line 480, The local variable wanted_node is
assigned from node in Hover.cpp but never used; remove the unused variable
assignment (the line creating wanted_node) or, if the intent was to operate on a
separate copy, replace uses of node with wanted_node where appropriate—search
for wanted_node and node in the Hover-related functions (e.g., any hover
handling logic) and either delete the wanted_node declaration or use it
consistently so the symbol is referenced.
src/Feature/Hover.cpp
Outdated
| std::optional<std::string> Hover::display(config::HoverOptions opt) { | ||
| std::string content; | ||
| llvm::raw_string_ostream os(content); | ||
| // TODO: generate markdown | ||
| StructedText out{}; | ||
| StructedText head_segment{}; | ||
| head_segment.add_heading(3).append_text(std::format("{}: {}", this->kind.name(), this->name)); | ||
| // TODO: provider | ||
| head_segment.add_ruler(); | ||
| // FIX: Delete later | ||
| head_segment.add_paragraph() | ||
| .append_text(std::format("Contains {} items", this->items.size())) | ||
| .append_newline_char(); | ||
| if(this->items.size()) { | ||
| auto& bl = head_segment.add_bullet_list(); | ||
| for(auto& hi: this->items) { | ||
| bl.add_item().add_paragraph().append_text( | ||
| std::format("{}: {}", clice::refl::enum_name(hi.kind), hi.value)); | ||
| } | ||
| } | ||
| out.append(head_segment); | ||
| if(this->document && opt.enable_doxygen_parsing) { | ||
| StructedText st{}; | ||
| auto& raw = *this->document; | ||
| auto [di, rest_] = strip_doxygen_info(raw); | ||
| if(auto rest = llvm::StringRef(rest_); !rest.trim().empty()) { | ||
| st.add_paragraph().append_text(rest.rtrim().str()); | ||
| st.add_ruler(); | ||
| } | ||
| auto bcc = di.get_block_command_comments(); | ||
| for(const auto [tag, content]: bcc) { | ||
| st.add_paragraph().append_text(tag.str(), Paragraph::Kind::Bold); | ||
| st.add_ruler(); | ||
| } | ||
| st.add_ruler(); | ||
| out.append(st); | ||
| } | ||
| if(!source.empty()) { | ||
| out.add_code_block(source + '\n', lang); | ||
| } | ||
| return out.as_markdown(); | ||
| } |
There was a problem hiding this comment.
Dead code and leftover debug artifacts in display().
- Line 693–694:
contentandosare created but never used. - Line 701:
// FIX: Delete later— the paragraph displaying item count (lines 702–704) is explicitly marked as temporary debug output that should be removed before merge.
Proposed cleanup
std::optional<std::string> Hover::display(config::HoverOptions opt) {
- std::string content;
- llvm::raw_string_ostream os(content);
- // TODO: generate markdown
StructedText out{};
StructedText head_segment{};
head_segment.add_heading(3).append_text(std::format("{}: {}", this->kind.name(), this->name));
// TODO: provider
head_segment.add_ruler();
- // FIX: Delete later
- head_segment.add_paragraph()
- .append_text(std::format("Contains {} items", this->items.size()))
- .append_newline_char();
if(this->items.size()) {🤖 Prompt for AI Agents
In `@src/Feature/Hover.cpp` around lines 692 - 733, In Hover::display remove the
unused debug variables and paragraph: delete the local variables "content" and
"os" (they are unused), and remove the temporary debug paragraph created after
the ruler (the head_segment.add_paragraph() call that appends "Contains {}
items" and the subsequent append_newline_char()), leaving the bullet-list logic
intact; ensure you also remove the "// FIX: Delete later" comment so the
function contains no leftover debug artifacts.
| if(command == "b" | command == "e" | command == "c") { | ||
| // Just start with inline command leave it. | ||
| goto normal_line; | ||
| } |
There was a problem hiding this comment.
Use logical OR (||) instead of bitwise OR (|).
Line 145 uses bitwise OR | between bool operands. While this happens to produce the correct result for bool, it prevents short-circuit evaluation and is almost certainly a typo. Use || for logical disjunction.
Proposed fix
- if(command == "b" | command == "e" | command == "c") {
+ if(command == "b" || command == "e" || command == "c") {📝 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.
| if(command == "b" | command == "e" | command == "c") { | |
| // Just start with inline command leave it. | |
| goto normal_line; | |
| } | |
| if(command == "b" || command == "e" || command == "c") { | |
| // Just start with inline command leave it. | |
| goto normal_line; | |
| } |
🤖 Prompt for AI Agents
In `@src/Support/Doxygen.cpp` around lines 145 - 148, The condition currently uses
bitwise OR between boolean expressions (if(command == "b" | command == "e" |
command == "c")) which prevents short-circuiting; update the conditional to use
logical OR (||) so comparisons short-circuit correctly and maintain intended
semantics for the check on the variable command in the same block that jumps to
normal_line.
| TEST_CASE(DeducedType_Auto) { | ||
| tester.clear(); | ||
| constexpr auto code = R"CODE( | ||
| struct A { | ||
| int a; | ||
| int b; | ||
| int c; | ||
| float d; | ||
| }; | ||
|
|
||
| struct A1 : A {}; | ||
|
|
||
| template <typename T> | ||
| struct B { | ||
| T *inner; | ||
|
|
||
| template <typename S> | ||
| B(S *s) { | ||
| inner = s; | ||
| } | ||
|
|
||
| template <typename S> | ||
| auto get_as() { | ||
| return *static_cast<S *>(inner); | ||
| } | ||
| }; | ||
|
|
||
| int main(void) { | ||
| au$(pos_0)to $(pos_0_i)a = A{}; | ||
| au$(pos_1)to $(pos_1_i)b = B<A>(&a); | ||
| au$(pos_2)to $(pos_2_i)a1 = b.get_as<A1>(); | ||
| au$(pos_3)to $(pos_3_i)msg = "hello world"; | ||
| au$(pos_4)to $(pos_4_i)c1 = 'c'; | ||
| au$(pos_5)to $(pos_5_i)c2 = 0; | ||
| au$(pos_6)to $(pos_6_i)c3 = 114514; | ||
| au$(pos_7)to $(pos_7_i)c4 = 11451419198101919810; | ||
| return 0; | ||
| } | ||
| )CODE"; | ||
| auto annotation = AnnotatedSource::from(code); | ||
| tester.add_main("main.cpp", annotation.content); | ||
| std::vector<unsigned> offsets; | ||
| tester.compile(); | ||
| ASSERT_TRUE(tester.unit.has_value()); | ||
| const unsigned count = annotation.offsets.size(); | ||
| for(unsigned i = 0; i < count / 2; ++i) { | ||
| unsigned offset = annotation.offsets[std::format("pos_{}", i)]; | ||
| auto HI = clice::feature::hover(*tester.unit, offset, {}); | ||
| std::println("Hover at pos_{}", i); | ||
| if(HI.has_value()) { | ||
| auto msg = HI->display({}); | ||
| std::println("```\n{}```\n", *msg); | ||
| } else { | ||
| std::println("No hover info"); | ||
| } | ||
| } | ||
|
|
||
| for(unsigned i = 0; i < count / 2; ++i) { | ||
| unsigned offset = annotation.offsets[std::format("pos_{}_i", i)]; | ||
| auto HI = clice::feature::hover(*tester.unit, offset, {}); | ||
| std::println("Hover at pos_{}_i", i); | ||
| if(HI.has_value()) { | ||
| auto msg = HI->display({}); | ||
| std::println("```\n{}```\n", *msg); | ||
| } else { | ||
| std::println("No hover info"); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Tests print results but assert nothing.
All the DeducedType_Auto, DeducedType_Decltype, and Comments test cases only call std::println with hover results. There are no ASSERT_* or EXPECT_* checks on the hover output, so these tests will always pass regardless of correctness. At minimum, assert that HI.has_value() is true for positions where hover is expected.
Also applies to: 186-260, 262-326
🤖 Prompt for AI Agents
In `@tests/unit/Feature/HoverTests.cpp` around lines 116 - 184, The test
DeducedType_Auto (and similar cases DeducedType_Decltype and Comments) only
prints hover results and never asserts correctness; update the loops that call
clice::feature::hover(*) storing the result in HI to include explicit assertions
(e.g., ASSERT_TRUE(HI.has_value()) before using HI->display()) for offsets where
hover info is expected (the annotated positions like pos_0..pos_7 and
pos_0_i..pos_7_i), and add additional ASSERT_EQ/ASSERT_NE on the returned
display string when specific text is expected to validate hover contents; ensure
you modify both loops that iterate over annotation.offsets and reference HI and
msg so the tests fail when hover is missing or incorrect.
2 participants
In process
Summary by CodeRabbit
New Features
API
Tests