This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Print arguments in template specializations
ClosedPublic

Authored by kadircet on Mar 14 2019, 3:59 AM.

Event Timeline

kadircet created this revision.Mar 14 2019, 3:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 3:59 AM
ilya-biryukov added inline comments.Mar 14 2019, 4:04 AM
clang-tools-extra/clangd/AST.cpp
59

For ClassTemplateSpecializationDecl, could we try extracting the arguments from the result of getTypeAsWritten?

clang/lib/AST/TypePrinter.cpp
1636

NIT: clang-format the diff

1644

Maybe print the result of getTypeLoc() here, if it's available?
Would produce results closer to the ones written in the code.

kadircet updated this revision to Diff 190617.Mar 14 2019, 7:15 AM
kadircet marked 3 inline comments as done.
  • Address comments
  • Add template specializations to fuzzyFind results
kadircet added inline comments.Mar 14 2019, 7:24 AM
clang/lib/AST/TypePrinter.cpp
1644

unfortunately it is not available.

kadircet updated this revision to Diff 190624.Mar 14 2019, 7:25 AM
  • Get rid of debug printing
  • Update tests

Mostly nits, but could you elaborate why can't we print the type-loc when printing the template argument (see the corresponding comment)?

clang-tools-extra/clangd/AST.cpp
66

NIT: add a note that we return null for ClassTemplateSpecializationDecl because we can't construct an ArrayRef for it

86

NIT: use SmallVector<8> or some other small-enough number to avoid most allocs.

clang-tools-extra/unittests/clangd/IndexTests.cpp
18

NIT: maybe remove it? clangd keeps adding those, but I don't think we actually want it: gmock.h should be enough

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
417

Does it mean typing bool could potentially match vector<bool> in workspaceSymbols now?
If that's the case we might start producing some noise.

clang/lib/AST/TypePrinter.cpp
1644

you mean the function to print a type loc or the type loc itself?

1646

Maybe simplify the switch to:

if (A.getKind() == TemplateArgument::ArgKind::Type) {
    A.getTypeSourceInfo()->getType().print(OS, PP);
    return;
}
A.getArgument().print(PP, OS);
kadircet marked 7 inline comments as done.Mar 14 2019, 9:00 AM
kadircet added inline comments.
clang-tools-extra/clangd/AST.cpp
86

calling reserve beforehand

clang-tools-extra/unittests/clangd/IndexTests.cpp
18
clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
417

unfortunately yes it does, what do you suggest? it seems like we can perform a "isprefix" check for symbols with template specs kind?

clang/lib/AST/TypePrinter.cpp
1644

function to print a typeloc

kadircet updated this revision to Diff 190642.Mar 14 2019, 9:00 AM
kadircet marked 2 inline comments as done.
  • Address some of the comments
kadircet marked an inline comment as done.Mar 14 2019, 9:36 AM
kadircet added inline comments.
clang/lib/AST/TypePrinter.cpp
1646

It was rather to catch any changes in the ArgKind at compile-time, still can do if you think this should not cause any problems

nridge added a subscriber: nridge.Mar 14 2019, 4:39 PM

The only important comment is about test coverage

clang-tools-extra/clangd/AST.cpp
86

SmallVector is even better because it keeps the elements on the stack and won't need to do dynamic allocations (most of the time).
My suggestion would be to use it here anyway.

Up to you, though.

clang-tools-extra/unittests/clangd/IndexTests.cpp
18

I think we should, but some projects don't like those pragmas in their headers.
Trying to add this won't hurt for sure!

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
417

Let's land this and see if this creates too much noise.
If it will, we might want to drop the argument lists from the words we match and only keep them for presentation to the user.

But let's not worry about this too much for now, the current behavior LG unless proven otherwise :-)

clang/lib/AST/TypePrinter.cpp
1646

I think listing all the cases here does not actually buy us anything since we call a function that should somehow print the arguments anyway.

But up to you.

1655

Now that you mentioned other kinds of parameters, could you add tests for other kinds of parameters?
In particular, I expect we'll definitely need to handle packs here and probably all other kinds of arguments too.

  • Non-type and template template parameters.
template <int I, int J, template<class> class TT, template <class> class JJ>
struct Foo {};

template <int X, template <class> class UU>
struct Foo<X, X, UU, UU> {};
  • Parameter packs.
template <class ...T>
struct Bar {};

template <class T>
struct Bar<T, T> {};

template <int ...I>
struct Baz {};

template <int I>
struct Baz<I, I> {};

template <template <class> class...TT>
struct Qux {};

template <template <class> class TT>
struct Qux<TT, TT> {};
ilya-biryukov added inline comments.Mar 15 2019, 3:33 AM
clang/lib/AST/TypePrinter.cpp
1644

Thanks for the clarification.
Intuitively, I've expected it to be there, but it seems that we can only print the types.

kadircet updated this revision to Diff 190800.Mar 15 2019, 3:55 AM
kadircet marked 11 inline comments as done.
  • Add more tests
  • Replace switch with if
  • Use SmallVector
clang/lib/AST/TypePrinter.cpp
1655

I've already had tests for these cases, but adding a few more for partial specs as well.

ilya-biryukov accepted this revision.Mar 19 2019, 3:15 AM

LGTM with a few NITs.
Thanks for fixing this!

clang-tools-extra/clangd/AST.cpp
85

NIT: maybe inline TL?

if (auto STL = Cls->getTypeAsWritten()->getTypeLoc().getAs<TemplateSpecializationTypeLoc>()) {
  /// ...
}
88
clang/lib/AST/TypePrinter.cpp
1635

NIT: consider also adding a test in the clang code.
Some developers don't build tools-extra.

This revision is now accepted and ready to land.Mar 19 2019, 3:15 AM
ilya-biryukov added inline comments.Mar 20 2019, 2:22 AM
clang/lib/AST/TypePrinter.cpp
1635

It seems the amount of test infrastructure for running this in clang might be overwhelming.
It's just not worth it probably, having a test in clangd is good enough!

kadircet updated this revision to Diff 191460.Mar 20 2019, 2:26 AM
kadircet marked 2 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.