[PATCH] D77598: Integral template argument suffix and cast printing

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/TemplateBase.cpp:71-72
 
   if (T->isBooleanType() && !Policy.MSVCFormatting) {
     Out << (Val.getBoolValue() ? "true" : "false");
   } else if (T->isCharType()) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:6825-6831
+    // Note: this renders CanonParamType non-canonical, but since every
+    // instantiation of this argument will be wrapped in an AutoType (since
+    // Param->getType() will always be an AutoType for this template), there
+    // should be no difference in how arguments are distinguished.
+    if (Param->getType()->getAs<AutoType>())
+      CanonParamType = Context.getAutoType(CanonParamType,
+                                           AutoTypeKeyword::Auto, false, false);
----------------
I'm uncomfortable with this approach. It's at least theoretically important that we canonicalize the types of template arguments so that template instantiations don't depend on how their arguments were spelled. Also, I don't think this addresses the whole problem: whether we need the type for a template argument depends not only on whether there was a deduced type in the parameter, but also whether the template was selected from an overload set. That's information that whoever is asking to print the template argument should know, but the template argument itself doesn't know in general.

So I'd like us to approach this a different way: change the calls into the template argument printing code to explicitly pass in a flag that says whether the type of the template argument is known from context. We should pass that in as `false` when:
1) dumping template arguments
2) the corresponding parameter contains a deduced type
3) the template arguments are for a `DeclRefExpr` that `hadMultipleCandidates()`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77598/new/

https://reviews.llvm.org/D77598