[PATCH] D140775: [clangd] Hover: show CalleeArgInfo for literals

[Nathan Ridge via Phabricator via cfe-commits]

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