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