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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a subscriber: jfb.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:95-96
+  case '?':
+  case 'n':
+  case 't':
+    return true;
----------------
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.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:422-423
 
-        ExprResult ArgExpr(
-            Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+        ExprResult ArgExpr(Actions.CorrectDelayedTyposInExpr(
+            ParseAttributeArgAsUnevaluatedLiteralOrExpression(AttrKind)));
         if (ArgExpr.isInvalid()) {
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Hmmm, I'm not certain about these changes.
> > 
> > For some attributes, the standard currently requires accepting any kind of string literal (like `[[deprecated]]` https://eel.is/c++draft/dcl.attr.deprecated#1). P2361 is proposing to change that, but it's not yet accepted by WG21 (let alone WG14). So giving errors in those cases is a bit of a hard sell -- I think warnings would be a bit more reasonable.
> > 
> > But for other attributes (like `annotate`), it's a bit less clear whether we should *prevent* literal prefixes because the attribute can be used to have runtime impacts (for example, I can imagine someone using `annotate` to emit the string literal bytes into the resulting binary). In some cases, I think it's very reasonable (e.g., `diagnose_if` should behave the same as `deprecated` and `nodiscard` because those are purely about generating diagnostics at compile time).
> > 
> > I kind of wonder whether we're going to want to tablegenerate whether the argument needs to be parsed as unevaluated or not on an attribute-by-attribute basis.
> Yep, I would not expect this to get merge before P2361 but I think the implementation experience is useful and raised a bunch of good questions.
> I don't think it ever makes sense to have `L` outside of literals - but people *might* do it currently, in which case there is a concern about whether it breaks code (I have found no evidence of that though).
> 
> If we wanted to inject these strings in the binary - in some form, then we might have to transcode them at that point.
> I don't think the user would know if the string would be injected as wide or narrow (or something else) by the compiler.
> 
> `L` is really is want to convert that string _at that point_. in an attribute, strings might have multiple usages so it's better to delay any transcoding.
> Does that make sense?
> 
> But I agree that a survey of what each attribute expects is in order.
> 
> 
> 
> Yep, I would not expect this to get merge before P2361 but I think the implementation experience is useful and raised a bunch of good questions.

Absolutely agreed, this is worthwhile effort!

> If we wanted to inject these strings in the binary - in some form, then we might have to transcode them at that point.
> I don't think the user would know if the string would be injected as wide or narrow (or something else) by the compiler.

My intuition is that a user who writes `L"foo"` will expect a wide `"foo"` to appear in the binary in the cases where the string ends up making it that far.

> L is really is want to convert that string _at that point_. in an attribute, strings might have multiple usages so it's better to delay any transcoding.
> Does that make sense?

Not yet, but I might get there eventually. :-D My concern is that vendor attributes can basically do anything, so there's no reason to assume that any given string literal usage should or should not transcode. I think we have to decide on a case by case basis by letting the attributes specify what they intend in their argument lists. However, my intuition is that *most* attributes will expect unevaluated string literals because the string argument doesn't get passed to LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759