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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:722
 
+  // Fill in value with initializer. Puts evaluated version if possible.
+  if (const auto *Var = dyn_cast<VarDecl>(D)) {
----------------
do we want the initializer both here and in Definition?

I suspect we'd like to turn off one or the other when both are present.

Looking at the examples, I think it's clearer to keep a non-evaluated initializer as part of the Definition (which is mostly as-written), and only include Value if it's evaluated.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:733
+        else
+          Init->printPretty(ValueOS, nullptr, Policy);
+      }
----------------
why not print the non-evaluated init if it's dependent?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:920
+      {R"cpp(
+        constexpr int add(int a, int b) { return a + b; }
+        int [[b^ar]] = add(1, 2);
----------------
constexpr may not be required here, I think?
At least the docs suggest evaluation ignores language semantics like this.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:956
+       }},
+      // FIXME: We should use TypeLoc instead of Decl to extract the concrete
+      // value.
----------------
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?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:981
+         HI.NamespaceScope = "";
+         HI.Value = "&\"1234\"[0]";
+       }},
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63330