Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 29385 Build 29384: arc lint + arc unit
Event Timeline
clang-tools-extra/clangd/AST.cpp | ||
---|---|---|
60 | 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? |
clang/lib/AST/TypePrinter.cpp | ||
---|---|---|
1644 | unfortunately it is not available. |
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 | ||
---|---|---|
67 | NIT: add a note that we return null for ClassTemplateSpecializationDecl because we can't construct an ArrayRef for it | |
87 | 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 | ||
440 | Does it mean typing bool could potentially match vector<bool> in workspaceSymbols now? | |
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); |
clang-tools-extra/clangd/AST.cpp | ||
---|---|---|
87 | calling reserve beforehand | |
clang-tools-extra/unittests/clangd/IndexTests.cpp | ||
18 | Should we add IWYU pragmas to those files? https://github.com/google/googlemock/blob/master/googlemock/include/gmock/gmock-generated-matchers.h | |
clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp | ||
440 | 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 |
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 |
The only important comment is about test coverage
clang-tools-extra/clangd/AST.cpp | ||
---|---|---|
87 | SmallVector is even better because it keeps the elements on the stack and won't need to do dynamic allocations (most of the time). 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. | |
clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp | ||
440 | Let's land this and see if this creates too much noise. 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?
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> {};
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> {}; |
clang/lib/AST/TypePrinter.cpp | ||
---|---|---|
1644 | Thanks for the clarification. |
- 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. |
LGTM with a few NITs.
Thanks for fixing this!
clang-tools-extra/clangd/AST.cpp | ||
---|---|---|
86 | NIT: maybe inline TL? if (auto STL = Cls->getTypeAsWritten()->getTypeLoc().getAs<TemplateSpecializationTypeLoc>()) { /// ... } | |
89 | Tiny NIT: prefer preincrement | |
clang/lib/AST/TypePrinter.cpp | ||
1635 | NIT: consider also adding a test in the clang code. |
clang/lib/AST/TypePrinter.cpp | ||
---|---|---|
1635 | It seems the amount of test infrastructure for running this in clang might be overwhelming. |
For ClassTemplateSpecializationDecl, could we try extracting the arguments from the result of getTypeAsWritten?