kadircet marked 15 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/AST.cpp:88
static const TemplateArgumentList *
getTemplateSpecializationArgs(const NamedDecl &ND) {
if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND))
----------------
ioeric wrote:
> can we unify this with `getTemplateSpecializationArgLocs` somehow?
>
> it seems that args as written would also be favorable here (see FIXME in line
> 112)
See D59641
================
Comment at: clang-tools-extra/clangd/AST.cpp:133
+ printTemplateArgumentList(OS, *Args, Policy);
+ else if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
+ if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {
----------------
ioeric wrote:
> why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?
it is mentioned in `getTemplateSpecializationArgLocs`, would you rather move
the comment here?
================
Comment at: clang/lib/AST/TypePrinter.cpp:1635
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+ llvm::raw_ostream &OS) {
----------------
ioeric wrote:
> could you add clang tests for these changes?
We've also discussed this in the original comment and decided infrastructure
necessary was too overwhelming. First we need to invoke compiler and get the
AST, then write a Visitor to fetch relevant decls and only after that call
printTemplateArguments ...
================
Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc &A,
+ const PrintingPolicy &PP, llvm::raw_ostream &OS) {
----------------
ioeric wrote:
> It's unclear to me what the new behavior is with changes in this file. Could
> you add comment?
>
> It might make sense to split the clang change into a separate patch and get
> folks who are more familiar to take a look.
it just prevents erasure from TypeLoc to Type when printing the argument, so
that we can do a fine-grained printing if Location is already there.
================
Comment at: clang/lib/AST/TypePrinter.cpp:1643
+ const auto &Kind = A.getArgument().getKind();
+ assert(Kind != TemplateArgument::Null &&
+ "TemplateArgumentKind can not be null!");
----------------
ioeric wrote:
> why?
>
> ```
> /// Represents an empty template argument, e.g., one that has not
> /// been deduced.
> ```
> It seems legitimate.
>
> Since this hot code path, we would want to avoid hard assertion if possible.
Sure, letting TemplateArgument::print handle that
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59639/new/
https://reviews.llvm.org/D59639
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
