[clang] Handle 128-bits IntegerLiterals in StmtPrinter

[davidstone]

Summary

This fixes PR35677: "int128_t or uint128_t as non-type template parameter causes crash when considering invalid constructor".

[riccibruno]

clang/lib/AST/StmtPrinter.cpp

1159

i128 and Ui128 are not valid integer literal suffix. The output of StmtPrinter is intended to be valid C++. Unfortunately here I think that your only choice is to print the high and low parts separately.

[davidstone]

clang/lib/AST/StmtPrinter.cpp

1159

I'm confused. i8, Ui8, i16, and Ui16 are also not valid C++ suffixes, but just a few lines up we use those. What am I missing here?

[riccibruno]

clang/lib/AST/StmtPrinter.cpp

1159

The literal suffixes [u]i8, [u]i16, [u]i32, and [u]i64 are an MSVC extension (see NumericLiteralParser::NumericLiteralParser in Lex/LiteralSupport.cpp).

This does not explain why they are used even in non-ms compatibility mode
but at least there is some reason for their existence.

However I don't think that MSVC supports 128-bits integers (?), and clang certainly
does not support [u]i128 so there is no reason to use them.

@aaron.ballman Do you know why are these suffixes used outside of ms-compatibility mode?

[riccibruno]

clang/lib/AST/StmtPrinter.cpp

1159

This does not explain why they are used even in non-ms compatibility mode
but at least there is some reason for their existence.

Let's just ask the author @majnemer

[aaron.ballman]

clang/lib/AST/StmtPrinter.cpp

1159

@aaron.ballman Do you know why are these suffixes used outside of ms-compatibility mode?

Our pretty printing is *supposed* to generate valid code, but we get it wrong fairly often and I don't think we've ever promised (let alone tested) that you can use this output to compile code (and get the same results). I think that's more of a stretch goal. That said, the pretty printer should probably be looking at the language options to decide whether it wants to output those suffixes or not.

As for historical context, I think the thread starting here is relevant: http://lists.llvm.org/pipermail/cfe-dev/2012-September/024423.html

[davidstone]

clang/lib/AST/StmtPrinter.cpp

1159

So what is the next step for this patch?

[aaron.ballman]

clang/lib/AST/StmtPrinter.cpp

1159

So what is the next step for this patch?

I don't think we should print any suffix for these, similar to how we handle int. There is no 128-bit suffix we support.

[riccibruno]

In addition to Aaron's suggestion, can you also change the diff's title to something a little bit more informative. Suggestion: "[clang] Handle 128-bits IntegerLiterals in StmtPrinter", and say in the description that this addresses PR35677.

[davidstone]

This comment was removed by davidstone.

[davidstone]

I believe this addresses the review comments.

[aaron.ballman]

LGTM, thanks!

[davidstone]

Reformatted

[aaron.ballman]

Do you need me to commit on your behalf? If so, which email address would you like me to use for attribution?

[davidstone]

In D82880#2651219, @aaron.ballman wrote:

Do you need me to commit on your behalf? If so, which email address would you like me to use for attribution?

Yeah, I do not have commit access. [e-mail], please

[aaron.ballman]

In D82880#2651698, @davidstone wrote:

In D82880#2651219, @aaron.ballman wrote:

Do you need me to commit on your behalf? If so, which email address would you like me to use for attribution?

Yeah, I do not have commit access. [e-mail], please

I've committed on your behalf in 4b5baa5b8244778b0e7253cdb98924c3dab611b7, thank you for the patch!

clang/lib/AST/StmtPrinter.cpp

void StmtPrinter::VisitIntegerLiteral(IntegerLiteral *Node) {
   case BuiltinType::Short:     OS << "i16"; break;
   case BuiltinType::UShort:    OS << "Ui16"; break;
   case BuiltinType::Int:       break; // no suffix.
   case BuiltinType::UInt:      OS << 'U'; break;
   case BuiltinType::Long:      OS << 'L'; break;
   case BuiltinType::ULong:     OS << "UL"; break;
   case BuiltinType::LongLong:  OS << "LL"; break;
   case BuiltinType::ULongLong: OS << "ULL"; break;
+  case BuiltinType::Int128:
+    OS << "i128";
+    break;
+  case BuiltinType::UInt128:
+    OS << "Ui128";
+    break;
   }
 }
 
 void StmtPrinter::VisitFixedPointLiteral(FixedPointLiteral *Node) {
   if (Policy.ConstantsAsWritten && printExprAsWritten(OS, Node, Context))
     return;
   OS << Node->getValueAsString(/*Radix=*/10);
 

clang/test/AST/ast-print-int128.cpp

+// RUN: %clang_cc1 -ast-print -std=c++20 %s -o - | FileCheck %s
+
+template <bool>
+struct enable_if {
+};
+
+template <__uint128_t x, typename = typename enable_if<x != 0>::type>
+void f();
+
+template <__int128_t>
+void f();
+
+using T = decltype(f<0>());
+
+// CHECK: using T = decltype(f<0>());