[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:110
+        break;
+      default:
+        if (T->isUnsignedIntegerType() && T->isWideCharType())
----------------
I think we should use the prettier printing for `wchar_t` / `char8_t` / `char16_t` / `char32_t` regardless of whether `IncludeType` is set, just like we do for `char`.


================
Comment at: clang/lib/AST/TemplateBase.cpp:112
+        if (T->isUnsignedIntegerType() && T->isWideCharType())
+          Out << "L'" << Val << "'";
+        else if (T->isUnsignedIntegerType() && T->isChar8Type()) {
----------------
This has the same problem that `u8` handling had: it will print the numeric value not the character.


================
Comment at: clang/lib/AST/TemplateBase.cpp:118-120
+        } else if (T->isUnsignedIntegerType() && T->isChar16Type())
+          Out << "u'" << Val << "'";
+        else if (T->isUnsignedIntegerType() && T->isChar32Type())
----------------
rsmith wrote:
> These two cases have the same problem that `u8` handling had: they will print the numeric value not the character.
You don't need the `isUnsignedIntegerType()` checks here (4x).


================
Comment at: clang/lib/AST/TemplateBase.cpp:118-121
+        } else if (T->isUnsignedIntegerType() && T->isChar16Type())
+          Out << "u'" << Val << "'";
+        else if (T->isUnsignedIntegerType() && T->isChar32Type())
+          Out << "U'" << Val << "'";
----------------
These two cases have the same problem that `u8` handling had: they will print the numeric value not the character.


================
Comment at: clang/lib/AST/TemplateBase.cpp:128
+    } else
+      Out << Val;
+  } else
----------------
Assuming this is reachable, we should include a cast here.


================
Comment at: clang/lib/AST/TemplateBase.cpp:442-450
+    // FIXME: Do not always include the type.
+    // For eg. the -ast-print of the following example -
+    // struct A {
+    //  template<long, auto> void f();
+    // };
+    // void g(A a) {
+    //  a.f<0L, 0L>();
----------------
In the expression case, we've not resolved the template argument against a parameter, so there's no possibility to include or not include the type. I would remove this FIXME.


================
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 {
----------------
rsmith wrote:
> 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.)
You need to pass in `I` here, or we'll assume that all elements of the pack correspond to the first template parameter.


================
Comment at: clang/test/CXX/lex/lex.literal/lex.ext/p12.cpp:21
 int f = UR"("тест 𐀀)"_x;
-int g = UR"("тест_𐀀)"_x; // expected-note {{in instantiation of function template specialization 'operator""_x<char32_t, 34, 1090, 1077, 1089, 1090, 95, 65536>' requested here}}
+int g = UR"("тест_𐀀)"_x; // expected-note {{in instantiation of function template specialization 'operator""_x<char32_t, (char32_t)34, (char32_t)1090, (char32_t)1077, (char32_t)1089, (char32_t)1090, (char32_t)95, (char32_t)65536>' requested here}}
----------------
rsmith wrote:
> It would be useful to generate `u8'...'`, `u16'...'`, and `u32'...'` literals at least whenever we think they'd contain printable characters.
Note the sample output here is wrong. We should ideally print

```
operator""_x<char32_t, U'\"', U'т', U'е', U'с', U'т', U'_', U'𐀀">
```

(with or without the `\` before the `"`), but if we don't have a good way to figure out which characters are printable, printing

```
operator""_x<char32_t, U'\"', U'\u0442', U'\u0435', U'\u0441', U'\u0442', U'_', U'\U00010000">
```

would be an acceptable fallback. You should check if LLVM already has some way to determine if a given Unicode character is printable and use it if available. I think the diagnostics infrastructure may use something like this already.


================
Comment at: clang/test/CXX/lex/lex.literal/lex.ext/p13.cpp:7
+template <typename T, T... str> int operator""_x() { // #1 expected-warning {{string literal operator templates are a GNU extension}}
+  check<T, str...> chars;                            // expected-error {{implicit instantiation of undefined template 'check<char8_t, 34, 209, 130, 208, 181, 209, 129, 209, 130, 95, 240, 144, 128, 128>'}}
+  return 1;
----------------
(Per earlier comment, I think we should use the prettier printing for `char8_t` here, regardless of `IncludeType`.)


================
Comment at: clang/test/SemaCXX/cxx1z-ast-print.cpp:8
+// CHECK: int k = TypeSuffix().x + TypeSuffix().y;
+int k = TypeSuffix().x<0L> + TypeSuffix().y<0L>; // expected-warning {{instantiation of variable 'TypeSuffix::x<0>' required here, but no definition is available}} expected-note {{add an explicit instantiation declaration to suppress this warning if 'TypeSuffix::x<0>' is explicitly instantiated in another translation unit}} expected-warning {{instantiation of variable 'TypeSuffix::y<0L>' required here, but no definition is available}} expected-note {{add an explicit instantiation declaration to suppress this warning if 'TypeSuffix::y<0L>' is explicitly instantiated in another translation unit}}
----------------
This line is unmanageably long; please move these `expected` comments to separate lines. (You can use `\` line continuation, or `expected-warning at -N`, or `expected-warning@#foo` to expect a diagnostic message on a different line.)


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

https://reviews.llvm.org/D77598