aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed.
Thanks for working on this! I think the direction is good in general, but I think we should also add tests for use in the preprocessor (`#if 1z == 1`, etc) as well as tests for the behavior in C. ================ Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:191 +def ext_cxx2b_size_t_suffix : Extension< + "size_t suffix for literals is a C++2b extension">, + InGroup<CXX2b>; ---------------- ================ Comment at: clang/include/clang/Lex/LiteralSupport.h:66 bool isLongLong : 1; + bool isSizeT : 1; // C++2b's z/uz size_t literals bool isHalf : 1; // 1.0h ---------------- ================ Comment at: clang/lib/Frontend/InitPreprocessor.cpp:593-595 + if (LangOpts.CPlusPlus2b) { + Builder.defineMacro("__cpp_size_t_suffix", "202011L"); + } ---------------- ================ Comment at: clang/lib/Lex/LiteralSupport.cpp:682 + break; // invalid for floats. + if (isMicrosoftInteger) + break; // invalid for Microsoft integers. ---------------- ================ Comment at: clang/lib/Lex/PPExpressions.cpp:325-327 + if (!PP.getLangOpts().CPlusPlus2b && Literal.isSizeT) { + PP.Diag(PeekTok, diag::ext_cxx2b_size_t_suffix); + } ---------------- ================ Comment at: clang/lib/Lex/PPExpressions.cpp:326 + if (!PP.getLangOpts().CPlusPlus2b && Literal.isSizeT) { + PP.Diag(PeekTok, diag::ext_cxx2b_size_t_suffix); + } ---------------- This feature is specific to C++ and needs some sort of diagnostic in C. There is not currently a WG14 proposal for this literal suffix, so I think this should be an error in that case rather than a warning. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3871-3873 + if (!getLangOpts().CPlusPlus2b && Literal.isSizeT) { + Diag(Tok.getLocation(), diag::ext_cxx2b_size_t_suffix); + } ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3872 + if (!getLangOpts().CPlusPlus2b && Literal.isSizeT) { + Diag(Tok.getLocation(), diag::ext_cxx2b_size_t_suffix); + } ---------------- Same diagnostic needs for C here as above. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3911 + if (Literal.isSizeT) { + assert(Ty.isNull() && "size_t literals can't be Microsoft literals"); + unsigned SizeTSize = Context.getTargetInfo().getTypeWidth( ---------------- The assert message doesn't seem to match the predicate -- why does a null qualtype imply it's a microsoft literal? ================ Comment at: clang/test/SemaCXX/size_t-literal.cpp:14-16 +#if __cplusplus >= 202101L +// expected-no-diagnostics +#endif ---------------- Rather than check `__cplusplus` like this, I think the RUN lines should specify a verify prefix. e.g., `-verify=cxx2b` and then use `cxx2b-no-diagnostics` and `-verify=cxx20` and then use `cxx20-warning {{}}`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99456/new/ https://reviews.llvm.org/D99456 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits