tom-anders 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) {
----------------
nridge wrote:
> 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`?
Changed back to the previous signature in current patch version
================
Comment at: clang-tools-extra/clangd/Hover.cpp:830
+ if (HI.CalleeArgInfo) {
+ HI.Name = "literal";
+ return HI;
----------------
nridge wrote:
> 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.
Added a FIXME for that
================
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);
----------------
nridge wrote:
> 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.
Good point, I refactored the logic here a bit, now CalleeArgInfo is added for
this branch, and also the two branches above.
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
