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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

A couple of small things, otherwise I'm happy; but Aaron has some bigger opens above, plus clang-format, plus the modules from Richard.



================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:257
+
+def err_unevaluated_string_prefix : Error<
+  "an unevaluated string literal cannot have an encoding prefix">;
----------------
aaron.ballman wrote:
> 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.
I was waffly on this too, so your waffling + my waffling I think is sufficient reason to not deal with this now.


================
Comment at: clang/lib/AST/Expr.cpp:1083
+
+  if (Kind != StringKind::Unevaluated) {
+    assert(Ctx.getAsConstantArrayType(Ty) &&
----------------
minor preference (perhaps 'nit' level) to move this whole CharByteWidth + IsPascal calculation into its own function.  This constructor is absurdly long as it is.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:98
+  case 'r':
+    return true;
+  }
----------------
For future clarification, the ones from the 'simple' list here: https://en.cppreference.com/w/cpp/language/escape

that we are missing are: `\a` `\b` `\f` and `\v`.

I personally think I'm ok with that until someone else says they care. 


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1541
+  Kind = tok::string_literal;
+  for (const auto &Tok : StringToks) {
+    // Unevaluated string literals can never have a prefix
----------------
aaron.ballman wrote:
> This means we're looping over (almost) all the string tokens three times -- once here, once below on line 1562, and again on 1605.
Hrm.... this is unfortunate.  Is there no way to combine the loops?  I guess (hope?) that hte list of tokens is at least going to be short...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759