[PATCH] D77598: Integral template argument suffix printing

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

rsmith added a comment.

In D77598#1990197 <https://reviews.llvm.org/D77598#1990197>, @rsmith wrote:

> Is is feasible to check whether the corresponding template parameter either has a deduced type or is a parameter of a function template? If not, we don't need to clarify the type of the template argument and could leave off the suffix to shorten the printed argument.


Ping on this comment. If possible, I'd prefer for us to not produce suffixes and casts in the case where there's no need to clarify the type of the template argument. I would imagine we can pass in a flag into the printing routine to indicate whether it needs to produce an expression with the right type, or merely one convertible to the right type.



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


================
Comment at: clang/lib/AST/TemplateBase.cpp:80
+    if (T->isBuiltinType()) {
+      switch (cast<BuiltinType>(T)->getKind()) {
+      case BuiltinType::ULongLong:
----------------
This isn't correct. Even if the type is canonically a `BuiltinType`, `T` might be a type sugar node, in which case this cast will fail at runtime. Instead, use:

```
if (auto *BT = T->getAs<BuiltinType>()) {
  switch (BT->getKind()) {
```

or similar.


================
Comment at: clang/lib/AST/TemplateBase.cpp:97-98
+      default:
+        if (T->isUnsignedIntegerType())
+          Out << Val << "U";
+        else
----------------
This isn't really appropriate for any of the remaining types other than `unsigned int`. If the type is unsigned but not `unsigned int` nor any of the cases you handled above, then a `U` suffix will never produce the right type (a decimal literal with a `U` suffix always has type `unsigned int`, `unsigned long`, or `unsigned long long`.)

Please convert this to a regular `case BuiltinType::UInt:` and use a cast for all the remaining cases.


================
Comment at: clang/lib/AST/TemplateBase.cpp:102
+              << Val;
+      }
+    } else
----------------
Tiny style nit: I'd like to see a `break;` here.


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

https://reviews.llvm.org/D77598