ilya-biryukov added a comment.
The only important comment is about test coverage
================
Comment at: clang-tools-extra/clangd/AST.cpp:84
+ if (auto STL = TL.getAs<TemplateSpecializationTypeLoc>()) {
+ std::vector<TemplateArgumentLoc> ArgLocs;
+ for (unsigned I = 0; I < STL.getNumArgs(); I++)
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > NIT: use `SmallVector<8>` or some other small-enough number to avoid most
> > allocs.
> calling reserve beforehand
`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.
================
Comment at: clang-tools-extra/unittests/clangd/IndexTests.cpp:18
#include "clang/Index/IndexSymbol.h"
+#include "gmock/gmock-generated-matchers.h"
#include "gmock/gmock.h"
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > NIT: maybe remove it? clangd keeps adding those, but I don't think we
> > actually want it: `gmock.h` should be enough
> Should we add IWYU pragmas to those files?
> https://github.com/google/googlemock/blob/master/googlemock/include/gmock/gmock-generated-matchers.h
I think we should, but some projects don't like those pragmas in their headers.
Trying to add this won't hurt for sure!
================
Comment at: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp:417
+ ForCodeCompletion(true)),
+ AllOf(QName("Tmpl<int, bool, Bar, 3>"),
+ DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Does it mean typing `bool` could potentially match `vector<bool>` in
> > `workspaceSymbols` now?
> > If that's the case we might start producing some noise.
> unfortunately yes it does, what do you suggest? it seems like we can perform
> a "isprefix" check for symbols with template specs kind?
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 :-)
================
Comment at: clang/lib/AST/TypePrinter.cpp:1646
+ break;
+ case TemplateArgument::ArgKind::Type:
+ A.getTypeSourceInfo()->getType().print(OS, PP);
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Maybe simplify the switch to:
> > ```
> > if (A.getKind() == TemplateArgument::ArgKind::Type) {
> > A.getTypeSourceInfo()->getType().print(OS, PP);
> > return;
> > }
> > A.getArgument().print(PP, OS);
> > ```
> >
> 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
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.
================
Comment at: clang/lib/AST/TypePrinter.cpp:1655
+ case TemplateArgument::ArgKind::Expression:
+ case TemplateArgument::ArgKind::Pack:
+ A.getArgument().print(PP, OS);
----------------
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> {};
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59354/new/
https://reviews.llvm.org/D59354
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
