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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Implementation LG though please do check that the new test results look useful and not too duplicative



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:723
+  // Fill in value with evaluated initializer if possible.
+  if (const auto *Var = dyn_cast<VarDecl>(D)) {
+    if (const Expr *Init = Var->getInit()) {
----------------
Maybe add a FIXME about doing this for arbitrary expressions (like sizeof and function calls), not just VarDecl?


================
Comment at: clang-tools-extra/clangd/XRefs.h:106
   llvm::Optional<std::vector<Param>> TemplateParameters;
+  /// Contains the evaluated value of the symbol if exists.
+  llvm::Optional<std::string> Value;
----------------
if exists -> if available?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:956
+       }},
+      // FIXME: We should use TypeLoc instead of Decl to extract the concrete
+      // value.
----------------
kadircet wrote:
> 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.
OK, let's leave this to later. Can you change the FIXME comment? I think it's misleading as it stands


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:981
+         HI.NamespaceScope = "";
+         HI.Value = "&\"1234\"[0]";
+       }},
----------------
kadircet wrote:
> 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)?
Let's leave it for now and see if it actually proves annoying.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:673
          HI.Type = "Foo<int, char, bool>";
+         HI.Value = "Foo<int, char, bool>(5)";
        }},
----------------
are these actually still emitted, or tests not updated?

Some of them (e.g. the lambda below!) don't seem useful


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63330