[PATCH] D77598: Integral template argument suffix and cast printing
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 9 13:36:27 PST 2020
rsmith added inline comments.
================
Comment at: clang/include/clang/AST/DeclTemplate.h:213
+ else if (Idx >= TPL->size())
+ return false;
+ else {
----------------
We should return `true` in this case; we don't know what the corresponding parameter is so we don't know if the type matters.
================
Comment at: clang/include/clang/AST/DeclTemplate.h:216
+ const NamedDecl *TemplParam = TPL->getParam(Idx);
+ if (auto *ParamValueDecl = dyn_cast<ValueDecl>(TemplParam))
+ if (ParamValueDecl->getType()->getContainedDeducedType())
----------------
May as well go straight to `NonTypeTemplateParamDecl` here; it can't be any other kind of `ValueDecl`.
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:48-72
+ const TemplateParameterList *TPL = nullptr;
+ if (auto SD = dyn_cast<DeclRefExpr>(S))
+ if (!SD->hadMultipleCandidates())
+ if (auto *TD = dyn_cast<TemplateDecl>(D))
+ TPL = TD->getTemplateParameters();
if (auto Args = D->getTemplateSpecializationArgs()) {
std::string ArgString;
----------------
I'm not entirely sure what this is used for, but it looks like the goal is uniqueness, not human-readability, given that the template arguments are separated by newlines not by commas or similar. That being the case, I think we should unconditionally pass `true` as `IncludeType` here.
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:61-64
+ } else if (i >= TPL->size()) {
+ Args->get(i).print(Context.getLangOpts(), OS, /*IncludeType*/ false);
+ }
+ else {
----------------
Nit: exactly one space between `}` and `else`. (You have two spaces in the first instance here and a newline in the second instance.)
================
Comment at: clang/lib/AST/ASTContext.cpp:7462
+ OS, TemplateArgs.asArray(), getPrintingPolicy(),
+ Spec->getSpecializedTemplate()->getTemplateParameters());
}
----------------
Pass `nullptr` here; for ObjC `@encode` we presumably want fully-explicit output always.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:651-652
+ ->getTemplateParameters();
+ // FIXME: Check if function template is overloaded
+ bool TemplOverloaded = false;
if (TArgAsWritten && !Policy.PrintCanonicalTypes)
----------------
We should conservatively assume that it is, rather than potentially printing out an ambiguous AST fragment.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:994-995
Args = TST->template_arguments();
- printTemplateArguments(Args);
+ // FIXME: Check if function template is overloaded
+ bool TemplOverloaded = false;
+ printTemplateArguments(
----------------
This is a class template specialization, not a function template specialization. It can't be overloaded.
================
Comment at: clang/lib/AST/DeclTemplate.cpp:1437
OS << "<";
+ // FIXME: Only pass parameter if template is not overloaded
+ // FIXME: Find corresponding parameter for argument
----------------
Concepts can't be overloaded; remove this FIXME.
================
Comment at: clang/lib/AST/Expr.cpp:702
TOut << Param << " = ";
- Args.get(i).print(Policy, TOut);
+ // FIXME: Only pass parameter if template is overloaded
+ Args.get(i).print(
----------------
Remove this FIXME: class template specializations can't be overloaded.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:1450
+ if (auto *FD = dyn_cast<FunctionDecl>(Node->getMemberDecl()))
+ TPL = FD->getDescribedTemplateParams();
+ else if (auto *TD =
----------------
This isn't right: `getDescribedTemplateParams` handles the case where the declaration is the pattern in a template definition, not where it's a template instantiation. What you want here is:
```
if (auto *FTD = FD->getPrimaryTemplate())
TPL = FTD->getTemplateParameters();
```
You should also take into account `Node->hadMultipleCandidates()` here.
Testcase for this would be something like:
```
struct A {
template<long, auto> void f();
};
void g(A a) { a.f<0L, 0L>(); }
```
... for which `-ast-print` should produce `f<0, 0L>`.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:1453
+ dyn_cast<VarTemplateSpecializationDecl>(Node->getMemberDecl()))
+ TPL = TD->getDescribedTemplateParams();
if (Node->hasExplicitTemplateArgs())
----------------
This is likewise not right. In this case we always know there's a template, so it's slightly simpler:
```
TPL = TD->getSpecializedTemplate()->getTemplateParameters();
```
Please also rename `TD` to something more appropriate.
Testcase for this would be something like:
```
struct A {
template<long> static int x;
template<auto> static int y;
};
int k = A().x<0L> + A().y<0L>;
```
... for which `-ast-print` should produce `x<0>` but `y<0L>`.
================
Comment at: clang/lib/AST/TemplateBase.cpp:54
+///
+/// \param IncludeType the flag to determine printing type information.
+static void printIntegral(const TemplateArgument &TemplArg, raw_ostream &Out,
----------------
I don't think this is very clear about the purpose of the flag. How about:
```
\param IncludeType If set, ensure that the type of the expression printed matches the type of the template argument.
```
================
Comment at: clang/lib/AST/TemplateBase.cpp:111-115
+ Out << "u8'" << Val << "'";
+ else if (T->isUnsignedIntegerType() && T->isChar16Type())
+ Out << "u16'" << Val << "'";
+ else if (T->isUnsignedIntegerType() && T->isChar32Type())
+ Out << "u32'" << Val << "'";
----------------
This isn't correct: `u8'x'` will print as `u8'120'`. Perhaps you can factor code to do this properly out of `StmtPrinter::VisitCharacterLiteral`.
Also, the prefix for `char16_t` literals is `u`, not `u16`, and the prefix for `char32_t` literals is `U`, not `u32`.
================
Comment at: clang/lib/AST/TemplateBase.cpp:79-84
+ if (IncludeType) {
+ if (T->isSignedIntegerType())
+ Out << "(signed char)";
+ else
+ Out << "(unsigned char)";
+ }
----------------
rsmith wrote:
> We should not include a cast for plain `char`, only for `signed char` and `unsigned char`.
This is marked 'done' but not done. Note that plain `char` is always either a signed or unsigned type, so your check here does not work. You can check `T->isSpecificBuiltinType(BuiltinType::SChar)` and `T->isSpecificBuiltinType(BuiltinType::UChar)` instead.
================
Comment at: clang/lib/AST/TemplateBase.cpp:395
case Declaration: {
NamedDecl *ND = getAsDecl();
----------------
rsmith wrote:
> Please add a FIXME to handle `knownType` here.
This is marked as done but not done. (The template parameter object case has a FIXME but the other cases do not and should.)
================
Comment at: clang/lib/AST/TemplateBase.cpp:71-72
if (T->isBooleanType() && !Policy.MSVCFormatting) {
Out << (Val.getBoolValue() ? "true" : "false");
} else if (T->isCharType()) {
----------------
rsmith wrote:
> rnk wrote:
> > rsmith wrote:
> > > reikdas wrote:
> > > > rsmith wrote:
> > > > > rsmith wrote:
> > > > > > It looks like `MSVCFormatting` wants `bool` values to be printed as `0` and `1`, and this patch presumably changes that (along with the printing of other builtin types). I wonder if this is a problem in practice (eg, if such things are used as keys for debug info or similar)...
> > > > > Do we need to suppress printing the suffixes below in `MSVCFormatting` mode too?
> > > > @rsmith The tests pass, so that is reassuring at least. Is there any other way to find out whether we need to suppress printing the suffixes for other types in MSVCFormatting mode?
> > > @rnk Can you take a look at this? Per rG60103383f097b6580ecb4519eeb87defdb7c05c9 and PR21528 it seems like there might be specific requirements for how template arguments are formatted for CodeView support; we presumably need to make sure we still satisfy those requirements.
> > Probably. This change looks like it preserves behavior from before when `MSVCFormatting` is set, so I think this is OK. I checked, my version of MSVC still uses 1/0 in debug info for boolean template args.
> My concern is that we're changing the behavior for the other cases below in `MSVCFormatting` mode, not that we're changing the behavior for `bool`. If we have specific formatting requirements in `MSVCFormatting` mode, they presumably apply to all types, not only to `bool`, so we should be careful to not change the output in `MSVCFormatting` mode for any type.
@rnk Ping.
================
Comment at: clang/lib/AST/TypePrinter.cpp:1980
bool FirstArg = true;
+ unsigned i = 0;
for (const auto &Arg : Args) {
----------------
Please capitalize this, following the naming convention of the surrounding code.
================
Comment at: clang/lib/AST/TypePrinter.cpp:1989
OS << Comma;
- printTo(ArgOS, Argument.getPackAsArray(), Policy, true, nullptr);
+ printTo(ArgOS, Argument.getPackAsArray(), Policy, true, TPL);
} else {
----------------
This is wrong; we'll incorrectly assume that element `i` of the pack corresponds to element `i` of the original template parameter list. We should use the same template parameter for all elements of the pack. (For example, you could pass in `I` and instruct the inner invocation of `printTo` to not increment `I` as it goes.)
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:992
TemplateArgumentListInfo &Args,
+ const TemplateParameterList *Params,
unsigned DiagID) {
----------------
It doesn't make sense to pass a parameter list in here, because we've not selected a template yet.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1035
Loc, TraitTy, DiagID,
- printTemplateArgs(S.Context.getPrintingPolicy(), Args));
+ printTemplateArgs(S.Context.getPrintingPolicy(), Args, Params));
return true;
----------------
In this case, the parameter list is `TraitTD->getTemplateParameters()`.
================
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);
----------------
rsmith wrote:
> 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`.
Ping, can this be replaced by a call to `printTemplateArgumentList`?
================
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
----------------
rsmith wrote:
> In this case, including substituted default argument values seems important.
Sorry, I wasn't clear: I think we should not include the template parameters here, so that we do print out substituted template arguments even if they're equivalent to the default argument.
================
Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp:471
+ template <auto... N> struct X {};
+ X<1, 1u>::type y; // expected-error {{no type named 'type' in 'TypeSuffix::X<1, 1u>'}}
+ X<1, 1>::type z; // expected-error {{no type named 'type' in 'TypeSuffix::X<1, 1>'}}
----------------
reikdas wrote:
> @rsmith This test fails and we are unable to print the suffix here because the length of the TemplateParameterList is less than the length of the corresponding list of Template Arguments. Do you have any suggestions on how we can fix this?
Once we hit a template parameter pack, we should use the same parameter for all subsequent arguments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77598/new/
https://reviews.llvm.org/D77598
More information about the cfe-commits
mailing list