[PATCH] D77598: Integral template argument suffix and cast printing
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 5 14:14:49 PST 2020
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
More information about the cfe-commits
mailing list