[PATCH] D105759: [WIP] Implement P2361 Unevaluated string literals
Erich Keane via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 27 08:33:38 PDT 2021
erichkeane added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:257
+
+def err_unevaluated_string_prefix : Error<
+ "an unevaluated string literal cannot have an encoding prefix">;
----------------
Is there value to combining these two diagnostics with a %select?
================
Comment at: clang/lib/Lex/LiteralSupport.cpp:108
+ unsigned CharWidth, DiagnosticsEngine *Diags,
+ const LangOptions &Features, bool Unevaluated) {
const char *EscapeBegin = ThisTokBuf;
----------------
This is like the 3rd time we're using 'Unevaluated' as a bool parameter. I have a pretty strong preference for making it a scoped-enum in 'Basic' somewhere.
================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1813
+ Diags->Report(TokLoc, UnevaluatedStringHasUDL
+ ? diag::err_unevaluated_string_udl
+ : diag::err_string_concat_mixed_suffix)
----------------
Is this OK? It looks like we're passing a ton of parameters to a diag type that doesn't have any wildcards?
================
Comment at: clang/lib/Lex/LiteralSupport.cpp:95-96
+ case '?':
+ case 'n':
+ case 't':
+ return true;
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > Do you intend to miss a bunch of escapes like `\'` and `\r` (etc)?
> > > \' is there. I am less sure about '\r' and '\a'. for example. This is something I realized after writing P2361.
> > > what does '\a` in static assert mean? even '\r' is not so obvious
> > Looking at the list again, I think only `\a` is really of interest here. I know some folks like @jfb have mentioned that `\a` could be used to generate an alert sound on a terminal, which is a somewhat useful feature for a failed static assertion if you squint at it hard enough.
> >
> > But the rest of the missing ones do seem more questionable to support.
> @jfb and @cor3ntin -- any opinions on whether `\a` should be supported? My opinion is that it should be supported because it has some utility for anyone running the compiler from a command line, but it's a pretty weak opinion.
I might consider rejecting ANY character escape in the less-than-32 part of the table.
For consistency at least, I don't see value in allowing \a if we're rejecting layout things like \t.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105759/new/
https://reviews.llvm.org/D105759
More information about the cfe-commits
mailing list