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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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">;
----------------
erichkeane wrote:
> Is there value to combining these two diagnostics with a %select?
I waffled when doing this review, so it's funny you mention it. :-D

We could do: `an unevaluated string literal cannot %select{have an encoding prefix|be a user-defined literal}0` but there was just enough text in the `select` that I felt it wasn't critical to combine. But I don't feel strongly either way.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1813
+            Diags->Report(TokLoc, UnevaluatedStringHasUDL
+                                      ? diag::err_unevaluated_string_udl
+                                      : diag::err_string_concat_mixed_suffix)
----------------
erichkeane wrote:
> Is this OK?  It looks like we're passing a ton of parameters to a diag type that doesn't have any wildcards?  
Good catch! The first two are not helpful (the diag engine will silently ignore them), but the second two are for underlines in the diagnostic and are useful.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:95-96
+  case '?':
+  case 'n':
+  case 't':
+    return true;
----------------
erichkeane wrote:
> 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.
But that's just it, we're accepting `\t` and `\n` with this code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759