Page MenuHomePhabricator

[clangd] Populate detail field in document symbols
ClosedPublic

Authored by lightmelodies on Feb 15 2021, 10:17 PM.

Diff Detail

Event Timeline

lightmelodies created this revision.Feb 15 2021, 10:17 PM
lightmelodies requested review of this revision.Feb 15 2021, 10:17 PM

Thanks for doing this!

I think this behavior is a great starting point.

I suspect printing the decl is more verbose than desired in a lot of cases, at least avoiding repeating the identifier would be nice. e.g.
x | int x --> x | int
foo | void foo() --> foo | void()
bar | struct bar {} --> bar | struct
This can be tweaked later, the simple implementation seems like a big improvement already.
If you feel like it though, I think this is mostly just printing the type instead of the decl for ValueDecls.

When you upload snapshots, please prepare the diffs with full context -U9999 to make review easier. (the Arcanist arc tool will do this automatically)

clang-tools-extra/clangd/FindSymbols.cpp
201

nit: limit the scope of raw_string_ostream by putting it in an anonymous block.
It does not flush until destroyed (or explicitly flushed)

201

Can we pull out a helper function for this? getSymbolDetail or so?

For now it's just a few lines but I imagine it could grow as we refine the behavior.

clang-tools-extra/clangd/Hover.h
20 ↗(On Diff #323882)

this isn't a reasonable thing to expose & depend on from FindSymbols.cpp.

It's fine to add a copy into FindSymbols.cpp, especially since you want a slightly different policy anyway

clang-tools-extra/clangd/test/symbols.test
1

Can you add unit-tests for this feature in FindSymbolsTests.cpp?
Don't need to assert the detail for every symbol we test there, but adding a WithDetail matcher and adding it to a few nodes in the testcases would be useful. In particular, at least one that shows the getDescribedTemplateParams bit.

(It's nice to have a basic smoke-test here but unit-tests are where we try to cover more of the cases)

Thanks for your response. Yes decl is too verbose in many cases. In fact I have tried another concise version, but I can not decide which one is better. Unit test in FindSymbolsTests.cpp is also done, but I would like upload them after we reach an agreement on what behavior is prefered.

lightmelodies marked 3 inline comments as done.Feb 16 2021, 6:03 PM

This looks really good. Only thing i'd really think about changing in behavior is dropping the template params.
Please go ahead and upload the unittests!

(And thanks for including the diff context)

clang-tools-extra/clangd/FindSymbols.cpp
184

As a tradeoff between brevity and information, we might consider printing template but not the param list.

Particularly for functions this is useful, as template params are mostly types repeated in the signature:

- DenseMap | template <typename InputIt> void (const InputIt&, const InputIt&)
+ DenseMap | template void (const InputIt&, const InputIt&)

But I think this may be fine for classes as well:

- DenseMap | template <typename KeyT, typename ValueT, typename KeyInfoT = ...> class
+ DenseMap | template class
188

It might not makes sense to print the placeholder DependentType, types that contain undeduced auto, and maybe some other cases.

(This can definitely be a fixme comment for later - it'll be hard to find exactly the right cases in one pass)

192

What you have here is probably best, just thinking out loud...

we could also consider including the underlying type like = int but I suspect the difference between ValueDecl is too subtle and sometimes the types are really long anyway.

194

(if we drop the template params then I don't think we need to even print "template" in this case because concepts are always templates)

197

nit: either OS.flush(); return Detail; or equivalently return std::move(OS.str());

(Otherwise whether OS's buffer gets flushed into the right string or not is fragile, I believe relying on move elision)

Change behavior of template and add unit tests.

lightmelodies marked 4 inline comments as done.Feb 18 2021, 3:20 AM
sammccall accepted this revision.Feb 18 2021, 5:55 AM

Nice! This looks good to land as-is, I just have some suggestions where we may want to mark behavior to revisit later, and some places where we could trim the tests a bit.

Do you have commit access, or want me to land this for you? (I'd need an address for the commit)

clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
384

the void return type isn't really meaningful

may want a FIXME here that () or constructor() would probably be better

(no need to fix though)

392

for destructors I think we should probably suppress detail entirely - signature is always the same and it's clear from the name it's a destructor.

493

up to you, but we don't need to add WithDetail() assertions everywhere, probably just enough to ensure we cover the interesting cases.

We could leave out these ones, exportContext, noLocals etc that are really testing other things.

580–581

feels like this could benefit from being "specialization struct" or so, "specialization int" below etc.

Again, this is a possible FIXME comment, not something that needs to be done now.

629

(all these WithDetail("")s seem pretty redundant)

This revision is now accepted and ready to land.Feb 18 2021, 5:55 AM

Better printing for C++ constructor and destructor. Remove unnecessary test cases.

Nice! This looks good to land as-is, I just have some suggestions where we may want to mark behavior to revisit later, and some places where we could trim the tests a bit.

Do you have commit access, or want me to land this for you? (I'd need an address for the commit)

I don't have commit access yet, it will be appreciated if you can land this for me (just use lightmelodies(lightmelodies@outlook.com))

sammccall accepted this revision.Feb 18 2021, 7:33 AM

Fantastic, thank you!

clang-tools-extra/clangd/FindSymbols.cpp
189

I think equivalent and a little safer is

std::string ConstructorType = VD->getType().getAsString(P);
llvm::StringRef WithoutVoid = ConstructorType;
WithoutVoid.consume_front("void");
OS << WithoutVoid;

(I'll apply this when landing)

Fantastic, thank you!

Maybe WithoutVoid.consume_front("void ");

Fantastic, thank you!

Maybe WithoutVoid.consume_front("void ");

Oh wow, yes... sizeof() was really doing some heavy lifting :-)

This revision was automatically updated to reflect the committed changes.