[PATCH] D77598: Integral template argument suffix and cast printing
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 18 11:21:53 PST 2020
rsmith added inline comments.
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:67-75
+ if (auto *ParamTypeDecl = dyn_cast<TypeDecl>(TemplParam)) {
+ const clang::Type *ParamType = ParamTypeDecl->getTypeForDecl();
+ if (auto *autoT = ParamType->getAs<AutoType>()) {
+ if (autoT->getAs<DeducedType>()) {
+ IncludeType = true;
+ }
+ } else if (ParamType->getAs<DeducedType>()) {
----------------
This case can't happen. The only way that a template parameter can be a `TypeDecl` is if it's a `TemplateTypeParmDecl`, which represents ` TemplateTypeParmType`, never a `DeducedType`.
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:76
+ }
+ } else if (auto *ParamValueDecl = dyn_cast<ValueDecl>(TemplParam)) {
+ const clang::Type *ParamType = ParamValueDecl->getType().getTypePtr();
----------------
Check directly for a `NonTypeTemplateParamDecl` here.
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:77
+ } else if (auto *ParamValueDecl = dyn_cast<ValueDecl>(TemplParam)) {
+ const clang::Type *ParamType = ParamValueDecl->getType().getTypePtr();
+ if (auto *autoT = ParamType->getAs<AutoType>()) {
----------------
Don't use `getTypePtr()` here, just use `QualType`'s `operator->`. Eg: `IncludeType = ParamValueDecl->getType()->getContainedDeducedType();`.
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:78-84
+ if (auto *autoT = ParamType->getAs<AutoType>()) {
+ if (autoT->getAs<DeducedType>()) {
+ IncludeType = true;
+ }
+ } else if (ParamType->getAs<DeducedType>()) {
+ IncludeType = true;
+ }
----------------
You don't need to check for both of these; `AutoType` derives from `DeducedType`. Also, you're only looking for a top-level deduced type here; use `getContainedDeducedType()` instead to properly handle `auto*` and the like.
================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:131-135
+ if (const TemplateArgument *TA = get<TemplateArgument>()) {
+ TA->print(PP, OS, /*IncludeType*/ true);
+ } else if (const TemplateArgumentLoc *TAL = get<TemplateArgumentLoc>()) {
+ TAL->getArgument().print(PP, OS, /*IncludeType*/ true);
+ } else if (const TemplateName *TN = get<TemplateName>())
----------------
No braces here, please.
================
Comment at: clang/lib/AST/Decl.cpp:1630
const TemplateArgumentList &TemplateArgs = Spec->getTemplateArgs();
+ // FIXME: Only pass template parameter list if template isn't overloaded
printTemplateArgumentList(
----------------
Class templates are never overloaded.
================
Comment at: clang/lib/AST/Decl.cpp:2859
+ TPL = TD->getTemplateParameters();
+ // FIXME: Only pass template parameter list if template isn't overloaded
+ printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy, TPL);
----------------
We don't know in this case, so I think if we're not going to check, then we shouldn't pass the parameter list. This change as-is can lead to us printing out ambiguous names for function templates, both due to argument types and due to omitting arguments equivalent to default arguments.
================
Comment at: clang/lib/AST/NestedNameSpecifier.cpp:291
Record->printName(OS);
- printTemplateArgumentList(OS, Record->getTemplateArgs().asArray(),
- Policy);
+ // FIXME: Only pass parameter list if function template is overloaded
+ printTemplateArgumentList(
----------------
This is a class template, not a function template, so can't be overloaded. (Also I think this comment is backwards.)
================
Comment at: clang/lib/AST/NestedNameSpecifier.cpp:321
+ TPL = TD->getTemplateParameters();
+ // FIXME: Only pass parameter list if template is overloaded
+ printTemplateArgumentList(OS, SpecType->template_arguments(), InnerPolicy,
----------------
Type templates can't be overloaded. We need the types if and only if we didn't resolve the template name to a specific template decl, which matches what this patch does, so you can just remove the FIXMEs.
================
Comment at: clang/lib/AST/NestedNameSpecifier.cpp:330-332
+ const TemplateParameterList *TPL = nullptr;
+ if (auto *TD = dyn_cast<TemplateDecl>(Record))
+ TPL = TD->getTemplateParameters();
----------------
I think `Record` will always be null here, so this `dyn_cast` will crash. For a dependent template specialization type, I don't think there's any situation in which we can know what the template parameter list is, so we should always include the types for template arguments (that is, pass `nullptr` as the template parameter list).
================
Comment at: clang/lib/AST/StmtPrinter.cpp:999
+ const TemplateParameterList *TPL = nullptr;
+ // FIXME: Get list of corresponding template parameters
if (Node->hasExplicitTemplateArgs())
----------------
I don't think there can ever be such a list: because the scope is dependent, we don't know what template is being named.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:1011
+ // FIXME: Get list of corresponding template parameters
+ const TemplateParameterList *TPL = nullptr;
if (Node->hasExplicitTemplateArgs())
----------------
Similarly here, because the lookup is unresolved, the template parameter list is unknown.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:1451
+ const TemplateParameterList *TPL = nullptr;
+ // FIXME: Get list of corresponding template parameters
if (Node->hasExplicitTemplateArgs())
----------------
The cases you need to check for here are `VarTemplateSpecializationDecl` and `FunctionDecl`.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:2228
+ const TemplateParameterList *TPL = nullptr;
+ // FIXME: Get list of corresponding template parameters
if (Node->hasExplicitTemplateArgs())
----------------
We can never have such a list here due to the dependent scope.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:2244
+ const TemplateParameterList *TPL = nullptr;
+ // FIXME: Get list of corresponding template parameters
if (Node->hasExplicitTemplateArgs())
----------------
Likewise.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:2326
+ const TemplateParameterList *TPL = nullptr;
+ if (auto *DRE = dyn_cast<DeclRefExpr>(E))
+ if (!DRE->hadMultipleCandidates())
----------------
`E` is a `ConceptSpecializationExpr`. It can't be a `DeclRefExpr`. What you want here is `TPL = E->getNamedConcept()->getTemplateParameters()`.
================
Comment at: clang/lib/AST/TemplateBase.cpp:79-84
+ if (IncludeType) {
+ if (T->isSignedIntegerType())
+ Out << "(signed char)";
+ else
+ Out << "(unsigned char)";
+ }
----------------
We should not include a cast for plain `char`, only for `signed char` and `unsigned char`.
================
Comment at: clang/lib/AST/TemplateBase.cpp:439
- P.print(Policy, Out);
+ // FIXME: What is the corresponding parameter for an unpacked argument?
+ P.print(Policy, Out, IncludeType);
----------------
Passing in `IncludeType` seems appropriate here; I think you can remove the FIXME. Though make sure you have a test for things like:
```
template<auto ...N> struct X {};
X<1, 1u> x; // should be printed with type suffixes
```
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:281
+ TPL = TD->getTemplateParameters();
+ // FIXME: Only pass template parameter list if template is overloaded
+ printTemplateArgumentList(OS, TArgs->asArray(), getPrintingPolicy(), TPL);
----------------
Debug info shouldn't depend on such things. I think we should never pass the template parameter list here, so we always get complete debug information.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1200-1202
+ // FIXME: Only pass parameter list if template is not overloaded
+ printTemplateArgumentList(OS, Ty->template_arguments(), getPrintingPolicy(),
+ TPL);
----------------
Likewise here, I think we should always produce complete type names in debug info even though that will be more verbose than necessary to uniquely identify the type.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2107
+ TPL = TD->getTemplateParameters();
+ // FIXME: Only pass template parameter list if template is not overloaded
printTemplateArgumentList(OS, VTpl->getTemplateArgs().asArray(),
----------------
Likewise here
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:980-1007
+ if (!Params)
+ Arg.getArgument().print(PrintingPolicy, OS, /*IncludeType*/ true);
+ else if (i >= Params->size())
+ Arg.getArgument().print(PrintingPolicy, OS, /*IncludeType*/ false);
+ else {
+ bool IncludeType = false;
+ const NamedDecl *TemplParam = Params->getParam(i);
----------------
I think this is around the 6th time this code fragment has appeared in this patch. Would it be possible to factor this out? Perhaps into something like
```
static bool TemplateParameterList::shouldIncludeTypeForArgument(TemplateParameterList *TPL, unsigned Idx)`
```
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:3564
// This is a template variable, print the expanded template arguments.
- printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+ // FIXME: Only pass parameter list if template is not overloaded
+ printTemplateArgumentList(
----------------
This can't be overloaded; you can remove this FIXME.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4723-4726
+ if (!TPL)
+ Arg.print(S.getPrintingPolicy(), OS, /*IncludeType*/ true);
+ else if (i >= TPL->size())
+ Arg.print(S.getPrintingPolicy(), OS, /*IncludeType*/ false);
----------------
Hm, are we missing commas between these arguments?
Please consider whether this and some of the other repeated instances of this code can be replaced by calls to `printTemplateArgumentList`.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:591
+ const TemplateParameterList *TPL = Template->getTemplateParameters();
+ // FIXME: Only pass parameter list if template is not overloaded
printTemplateArgumentList(OS, Active->template_arguments(),
----------------
I don't think we should ever pass it in this case; we want to print out at least the values of prior arguments that were instantiated from default arguments.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:658-661
+ const TemplateParameterList *TPL = nullptr;
+ // FIXME: Only pass parameter list if template is not overloaded
+ if (auto *TD = dyn_cast<TemplateDecl>(Active->Entity))
+ TPL = TD->getTemplateParameters();
----------------
This one is debatable, but I think we should also never pass the argument list here, and always print out the full description of the function template specialization.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:814-818
+ if (!isa<FunctionDecl>(Active->Entity)) {
+ const TemplateParameterList *TPL = nullptr;
+ if (auto *TD = dyn_cast<TemplateDecl>(Active->Entity))
+ TPL = TD->getTemplateParameters();
+ // FIXME: Only pass parameter list if template is not overloaded
----------------
In this case, including substituted default argument values seems important.
================
Comment at: clang/test/CXX/lex/lex.literal/lex.ext/p12.cpp:21
int f = UR"("ัะตัั ๐)"_x;
-int g = UR"("ัะตัั_๐)"_x; // expected-note {{in instantiation of function template specialization 'operator""_x<char32_t, 34, 1090, 1077, 1089, 1090, 95, 65536>' requested here}}
+int g = UR"("ัะตัั_๐)"_x; // expected-note {{in instantiation of function template specialization 'operator""_x<char32_t, (char32_t)34, (char32_t)1090, (char32_t)1077, (char32_t)1089, (char32_t)1090, (char32_t)95, (char32_t)65536>' requested here}}
----------------
It would be useful to generate `u8'...'`, `u16'...'`, and `u32'...'` literals at least whenever we think they'd contain printable characters.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77598/new/
https://reviews.llvm.org/D77598
More information about the cfe-commits
mailing list