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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
