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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
