[PATCH] D99456: [C++2b] Support size_t literals

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:190
   InGroup<CXX98CompatPedantic>, DefaultIgnore;
+def ext_cxx2b_size_t_suffix : Extension<
+  "'size_t' suffix for literals is a C++2b extension">,
----------------
Should this be an `ExtWarn`? I think we should warn on this by default.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:594-595
 
   // Loop over all of the characters of the suffix.  If we see something bad,
   // we break out of the loop.
   for (; s != ThisTokEnd; ++s) {
----------------
General comment: I think the checks here have become too complex and error-prone. I suggest we add another flag, `hasSize`, that's set when we parse any size modifier (`F`, `L`, `LL`, `H`, `F128`, `I64`, and now `Z`), and when parsing any size modifier, replace the existing ad-hoc set of checks with a check for `hasSize`.


================
Comment at: clang/lib/Lex/PPExpressions.cpp:325
+    // 'z/uz' literals are a C++2b feature.
+    if (!PP.getLangOpts().CPlusPlus2b && Literal.isSizeT)
+      PP.Diag(PeekTok, PP.getLangOpts().CPlusPlus
----------------
We should issue a compat warning when these literals are used in C++2b or later. (That is, always issue a diagnostic for a `size_t` literal: in C, it's an error, in C++20 and before, it's an extension warning, and in C++2b and later, it's a backwards-compatibility warning.) There are a bunch of examples of this elsewhere in the codebase -- look for the emission of `warn_cxx.*_compat`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3871
+    // 'z/uz' literals are a C++2b feature.
+    if (!getLangOpts().CPlusPlus2b && Literal.isSizeT)
+      Diag(Tok.getLocation(), getLangOpts().CPlusPlus
----------------
Also need the C++20-and-before compat warning here for C++2b-onwards mode.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3928
+
       if (Ty.isNull() && !Literal.isLong && !Literal.isLongLong) {
         // Are int/unsigned possibilities?
----------------
This should exclude the `isSizeT` case in addition to `isLong` and `isLongLong`. (It probably doesn't matter, because `int` is not larger than `size_t` on any platform we support, but nonetheless we should check.)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3944
       // Are long/unsigned long possibilities?
       if (Ty.isNull() && !Literal.isLongLong) {
         unsigned LongSize = Context.getTargetInfo().getLongWidth();
----------------
Likewise here.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3975
       // Check long long if needed.
       if (Ty.isNull()) {
         unsigned LongLongSize = Context.getTargetInfo().getLongLongWidth();
----------------
Likewise here -- and this one actually does matter, because we have targets with a 32-bit `size_t` but a 64-bit `long long`.


================
Comment at: clang/test/Lexer/size_t-literal.cpp:6-8
+#if 1uz != 1
+#error "size_t suffix must be recognized by preprocessor"
+#endif
----------------
Please also check `-1z < 0` and `-1zu < 0` to ensure that the preprocessor gets the signedness correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99456