[PATCH] D63330: [clangd] Add Value field to HoverInfo

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:733
+        else
+          Init->printPretty(ValueOS, nullptr, Policy);
+      }
----------------
sammccall wrote:
> why not print the non-evaluated init if it's dependent?
not-necessary anymore, since we only generate value if it can be evaluated.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:956
+       }},
+      // FIXME: We should use TypeLoc instead of Decl to extract the concrete
+      // value.
----------------
sammccall wrote:
> Hmm, actually I'm not sure whether this is TypeLoc vs Decl here. Rather this should be a DeclRefExpr to the VarDecl (result) in the *expanded* CXXRecordDecl.
> 
> This suggests that maybe the isValueDependent check isn't quite right - the initializer expression is value dependent as spelled, but in the instantiation it's resolved. Not sure if this is fixable. What happens if you comment out the check?
yes, and the problem lies in the IndexingAPI, inside `handleDeclOccurence` implicit instantiations are resolved into templated decls. therefore we lose the template params :/

we can't call expression evaluator on value dependent expressions, there is an assertion in the beginning of EvaluateAsRvalue.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:981
+         HI.NamespaceScope = "";
+         HI.Value = "&\"1234\"[0]";
+       }},
----------------
sammccall wrote:
> whoa, that's... weird. But fine, I think.
> 
> (It would be nice to elide the `&[]` if the index is zero, or maybe print as `"1234" + 2` etc)
do you think it is worth changing that in printpretty (either the defaults or via some `PrintingPolicy` option)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63330/new/

https://reviews.llvm.org/D63330