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@-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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
