Page MenuHomePhabricator

[clangd] Print arguments in template specializations
ClosedPublic

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

Diff Detail

Repository
rC Clang

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
57 ↗(On Diff #190591)

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

clang/lib/AST/TypePrinter.cpp
1636 ↗(On Diff #190591)

NIT: clang-format the diff

1644 ↗(On Diff #190591)

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 ↗(On Diff #190591)

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 ↗(On Diff #190624)

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

84 ↗(On Diff #190624)

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

clang-tools-extra/unittests/clangd/IndexTests.cpp
18 ↗(On Diff #190624)

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 ↗(On Diff #190624)

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 ↗(On Diff #190591)

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

1646 ↗(On Diff #190624)

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
84 ↗(On Diff #190624)

calling reserve beforehand

clang-tools-extra/unittests/clangd/IndexTests.cpp
18 ↗(On Diff #190624)
clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
417 ↗(On Diff #190624)

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 ↗(On Diff #190591)

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 ↗(On Diff #190624)

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
84 ↗(On Diff #190624)

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 ↗(On Diff #190624)

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 ↗(On Diff #190624)

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 ↗(On Diff #190624)

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 ↗(On Diff #190642)

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 ↗(On Diff #190591)

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 ↗(On Diff #190642)

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
86 ↗(On Diff #190800)

NIT: maybe inline TL?

if (auto STL = Cls->getTypeAsWritten()->getTypeLoc().getAs<TemplateSpecializationTypeLoc>()) {
  /// ...
}
89 ↗(On Diff #190800)
clang/lib/AST/TypePrinter.cpp
1635 ↗(On Diff #190800)

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 ↗(On Diff #190800)

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.