nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:779
-HoverInfo getStringLiteralContents(const StringLiteral *SL,
- const PrintingPolicy &PP) {
- HoverInfo HI;
-
+void addStringLiteralContents(const StringLiteral *SL, const PrintingPolicy
&PP,
+ HoverInfo &HI) {
----------------
nit: "contents" seems a bit strange now that this is no longer necessarily the
entirety of the hover contents (nor is it the string literal's contents)
maybe name it `addStringLiteralInfo`?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:826
+ if (const StringLiteral *SL = dyn_cast<StringLiteral>(E)) {
+ addStringLiteralContents(SL, PP, HI);
+ return HI;
----------------
nit: for consistency with non-literal expressions, maybe the CalleeArgInfo
should come //after// the other contents of the hover?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:830
+ if (HI.CalleeArgInfo) {
+ HI.Name = "literal";
+ return HI;
----------------
tom-anders wrote:
> `HoverInfo::present` has an assertion that the `Name` has to be non-empty.
> I'm open for other name suggestions here (Or we could of course adjust
> `HoverInfo::present` instead)
It at least seems no worse than `"expression"` for other expressions.
I think the expression's value would be more useful (and despite the "not much
value" comment above, I think there //can// be value in showing this if e.g.
the expression is written as hex and the hover shows you the decimal value),
but that can be left for a later change.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:843
// evaluatable.
if (auto Val = printExprValue(E, AST.getASTContext())) {
HI.Type = printType(E->getType(), AST.getASTContext(), PP);
----------------
Any reason not to also add CalleeArgInfo in this branch?
I know this branch is not for literals so I'm suggesting to expand the scope of
the patch, feel free to leave this for later, but conceptually I don't see why
the CalleeArgInfo would be any less useful for non-literal expressions that
take this branch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140775/new/
https://reviews.llvm.org/D140775
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
