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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
