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

[Tom Praschan via Phabricator via cfe-commits]

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