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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:95-96
+  case '?':
+  case 'n':
+  case 't':
+    return true;
----------------
aaron.ballman wrote:
> 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.
Ah!  I missed that this is an allow-list instead of a deny-list.  That makes me way more comfortable with this code.

IMO, I'd suggest we we allow '\r' (since wouldn't we have problems on Windows at that point, being unable to accept a printable newline for windows?), but disallow `\a` for now unless someone comes up with a really good reason to allow it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759