[PATCH] D105759: [WIP] Implement P2361 Unevaluated string literals

[Erich Keane via Phabricator via cfe-commits]

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