[PATCH] D82880: Fix PR35677: UB on __int128_t or __uint128_t template parameters.

[David Stone via Phabricator via cfe-commits]

davidstone marked an inline comment as done.
davidstone added inline comments.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:1159
+  case BuiltinType::UInt128:
+    OS << "Ui128";
+    break;
----------------
aaron.ballman wrote:
> riccibruno wrote:
> > riccibruno wrote:
> > > davidstone wrote:
> > > > riccibruno wrote:
> > > > > `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. 
> > > > 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?
> > > 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?
> > > 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 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
So what is the next step for this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82880