[PATCH] D105759: Implement P2361 Unevaluated string literals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I don't think it's correct to assume that all string arguments to attributes are unevaluated, but it is hard to tell where to draw the line sometimes. Backing up a step, as I understand P2361 <https://reviews.llvm.org/P2361>, an unevaluated string is one which is not converted into the execution character set (effectively). Is that correct? If so, then as an example, `[[clang::annotate()]]` should almost certainly be using an evaluated string because the argument is passed down to LLVM IR and is used in ways we cannot predict. What's more, an unevaluated string cannot have some kinds of escape characters (numeric and conditional escape sequences) and those are currently allowed by `clang::annotate` and could potentially be used by a backend plugin.

I think other attributes may have similar issues. For example, the `alias` attribute is a bit of a question mark for me -- that takes a string literal representing an external identifier that is looked up. I'm not certain whether that should be in the execution character set or not, but we do support escape sequences for it: https://godbolt.org/z/v65Yd7a68

I think we need to track evaluated vs not on the argument level so that the attributes in Attr.td can decide which form to use. I think we should default to "evaluated" for any attribute we're on the fence about because that's the current behavior they get today (so we should avoid regressions).



================
Comment at: clang/include/clang/Sema/ParsedAttr.h:919
+  bool isStringLiteralArg(unsigned I) const {
+    // If the last bit is set, assume we have a variadic parameter
+    if (I >= StringLiterals.size())
----------------



================
Comment at: clang/lib/Parse/ParseDecl.cpp:291
 
+/// Determine whether the given attribute has an identifier argument.
+static ParsedAttributeArgumentsProperties
----------------
Comment doesn't match the function name. ;-)


================
Comment at: clang/lib/Parse/ParseDecl.cpp:453-454
+      ExprResult Expr = Actions.CorrectDelayedTyposInExpr(E);
+      if (Expr.isUsable())
+        E = Expr.get();
+    }
----------------
What are these lines intended to do? We assign to `E` but nothing ever reads from it after this assignment and we reset it on the next iteration through the loop.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3497
       Expr = ParseBraceInitializer();
-    } else
+    } else {
       Expr = ParseAssignmentExpression();
----------------
Can revert these two changes now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759