rsmith added a comment.
Having looked a bit more at the callers here, I think the pattern we're going
to want is:
- everywhere that prints a template argument list also asks for a list of
corresponding template parameters
- if the list of template parameters is unknown (perhaps because the template
name is dependent), or ambiguous from the printed context (because the template
was an overloaded function template), then always include types for template
arguments
- otherwise, include types for only template arguments where the parameter's
type involved a deduced type
On that basis, I think it would be preferable to change
`TemplateArgument::print` to accept not a `bool` indicating whether the type
was known, but instead a `const NamedDecl *` for the corresponding template
parameter. Then the logic is: include the type if we don't know the
corresponding parameter or if its type is deduced. And in the caller, we want
to always pass in the corresponding parameter if we know it, except when
printing the arguments for an overloaded call to a function template (for which
the types of the template arguments could affect which overload we selected).
[The comments below assume this refactoring is not done.]
================
Comment at: clang/include/clang/AST/StmtDataCollectors.td:54
for (unsigned i = 0; i < Args->size(); ++i) {
- Args->get(i).print(Context.getLangOpts(), OS);
+ Args->get(i).print(Context.getLangOpts(), OS, true);
// Add a padding character so that 'foo<X, XX>()' != 'foo<XX, X>()'.
----------------
We only know the type if the call wasn't overloaded.
================
Comment at: clang/include/clang/AST/TemplateBase.h:382
+ void print(const PrintingPolicy &Policy, raw_ostream &Out,
+ bool knownType) const;
----------------
Please capitalize this name as `KnownType` to be consistent with surrounding
code.
================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:132-136
+ bool knownType = true;
+ if (auto *DRE = dyn_cast<DeclRefExpr>(TA->getAsExpr()))
+ if (DRE->hadMultipleCandidates())
+ knownType = false;
+ TA->print(PP, OS, knownType);
----------------
Perhaps we should always request that the type is included when printing a
`DynTypedNode` -- someone printing a `DynTypedNode` can't be assumed to always
know the type of the node in general. But this really depends on the intent of
the caller -- if the purpose is to provide text that can be inserted by a
refactoring tool, whether we include type information or not will depend on the
context where the value is used, not only on the value itself.
I'd prefer that we either always pass in `false` (given that we don't know
whether the type is known in the caller's context) or always pass in `true` (to
preserve the old behavior).
================
Comment at: clang/lib/AST/DeclPrinter.cpp:1080
Out << ", ";
- Args[I].print(Policy, Out);
+ Args[I].print(Policy, Out, true);
}
----------------
What we should do for both this function and the one below is to pass in a flag
to indicate whether the template itself (not the template argument) was
overloaded, and also pass in the corresponding template parameter list. Then we
know the type only if the template was not overloaded and the parameter's type
does not contain a deduced type.
================
Comment at: clang/lib/AST/DeclTemplate.cpp:1429-1435
+ for (auto &ArgLoc : ArgsAsWritten->arguments()) {
+ bool knownType = true;
+ if (auto *DRE = dyn_cast<DeclRefExpr>(ArgLoc.getArgument().getAsExpr()))
+ if (DRE->hadMultipleCandidates())
+ knownType = false;
+ ArgLoc.getArgument().print(Policy, OS, knownType);
+ }
----------------
We should say we know the type here unless the corresponding template parameter
has a deduced type.
================
Comment at: clang/lib/AST/Expr.cpp:790
TOut << Param << " = ";
- Args->get(i).print(Policy, TOut);
+ Args->get(i).print(Policy, TOut, true);
TOut << ", ";
----------------
I think this should be `false`: generally we want `__PRETTY_FUNCTION__` and
friends to uniquely identify the function template specialization.
================
Comment at: clang/lib/AST/TemplateBase.cpp:54-56
+/// \param knownType the flag to determine printing suffix/prefix type.
+static void printIntegral(const TemplateArgument &TemplArg, raw_ostream &Out,
+ const PrintingPolicy &Policy, bool knownType) {
----------------
I'd find it much more intuitive to reverse the sense of this flag so that it's
an `IncludeType` flag or similar.
================
Comment at: clang/lib/AST/TemplateBase.cpp:78-81
const char Ch = Val.getZExtValue();
Out << ((Ch == '\'') ? "'\\" : "'");
Out.write_escaped(StringRef(&Ch, 1), /*UseHexEscapes=*/ true);
Out << "'";
----------------
We'll need to include a cast or similar here for `signed char` and `unsigned
char`.
================
Comment at: clang/lib/AST/TemplateBase.cpp:84-90
+ if (auto *autoT = T->getAs<AutoType>()) {
+ if (autoT->getAs<DeducedType>()) {
+ knownType = false;
+ }
+ } else if (T->getAs<DeducedType>()) {
+ knownType = false;
+ }
----------------
I don't think we can reliably determine whether the type was originally a
deduced type from here; a canonical `TemplateArgument` doesn't preserve that
information. Instead, we should determine whether the type of the corresponding
parameter contained a deduced type, in the callers of `TemplateArgument::print`.
================
Comment at: clang/lib/AST/TemplateBase.cpp:395
case Declaration: {
NamedDecl *ND = getAsDecl();
----------------
Please add a FIXME to handle `knownType` here.
================
Comment at: clang/lib/AST/TemplateBase.cpp:403
case NullPtr:
Out << "nullptr";
----------------
Please add a FIXME to handle `knownType` here.
================
Comment at: clang/lib/AST/TemplateBase.cpp:71-72
if (T->isBooleanType() && !Policy.MSVCFormatting) {
Out << (Val.getBoolValue() ? "true" : "false");
} else if (T->isCharType()) {
----------------
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.
================
Comment at: clang/lib/AST/TypePrinter.cpp:1789-1793
+ bool knownType = true;
+ if (auto *DRE = dyn_cast<DeclRefExpr>(A.getArgument().getAsExpr()))
+ if (DRE->hadMultipleCandidates())
+ knownType = false;
+ return A.getArgument().print(PP, OS, knownType);
----------------
We should pass a corresponding template parameter declaration in here, and pass
`true` for `KnownType` only if we know the corresponding parameter and it
doesn't contain a deduced type.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77598/new/
https://reviews.llvm.org/D77598
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
