[PATCH] D105759: Implement P2361 Unevaluated string literals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1411
   let Documentation = [DeprecatedDocs];
+  let ParseArgumentsAsUnevaluated = 1;
 }
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > What is the plan for non-standard attributes? Are you planning to handle those in a follow-up, or should we be investigating those right now?
> I don't feel I'm qualified to answer that. Ideally, attributes that expect string literals that are not evaluated should follow suite.
Let's do them in a follow-up. Normally I'd suggest working with @erichkeane on which attributes to apply that to, but he's about to go on a sabbatical and might not have time to help with that. So maybe you can take a first pass at it as best you can and then rope me in to help finalize it, if that'd work for you?


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:2080-2082
+      if (!isUnevaluated() && Features.PascalStrings &&
+          ThisTokBuf + 1 != ThisTokEnd && ThisTokBuf[0] == '\\' &&
+          ThisTokBuf[1] == 'p') {
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Is there test coverage that we diagnose this properly?
> What sort of test would you like to see?
Pascal strings enabled and using something like `[[deprecated("\pOh no, a Pascal string!")]]` (or some other unevaluated uses).


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3501-3503
+    } else if (!AllowEvaluatedString && tok::isStringLiteral(Tok.getKind())) {
+      Expr = ParseUnevaluatedStringLiteralExpression();
+    } else {
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > I'm surprised we need special logic in `ParseExpressionList()` for handling unevaluated string literals; I would have expected that to be needed when parsing a string literal. Nothing changed in the grammar for http://eel.is/c++draft/expr.post.general#nt:expression-list (or initializer-list), so these changes seem wrong. Can you explain the changes a bit more?
> We use `ParseExpressionList` when parsing attribute arguments, and some attributes have unevaluate string as argument - I agree with you that I'd rather find a better solution for attributes, but I came up empty. There is no further reason for this change, and you are right it does not match the grammar.
I was thinking we'd use a new kind of evaluation context for this. We'd enter the evaluation context when we know we need to parse an expression that is an unevaluated string literal which the string literal parser would pay attention to. This would require knowing up-front when we want to parse an unevaluated string literal, but we should have that information available to us at parse time (I think).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:878-879
 
-  if (!isIntOrBool(AL.getArgAsExpr(0))) {
+  Expr *First = AL.getArgAsExpr(0);
+  if (isa<StringLiteral>(First) || !isIntOrBool(First)) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Test coverage for these changes?
> There is one somewhere, I don;t remember where, The reason we need to do that is that Unevaluated StringLiterals don''t have types
Let's try to track that down, but... an unevaluated string literal still has a type, surely? It'd be `const char[]` for C++?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759