[PATCH] D105759: Implement P2361 Unevaluated string literals

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3048
   }];
+  let ParseArgumentsAsUnevaluated = 1;
 }
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > I don't think we should reuse this flag this way. This flag is for the traditional sense of "unevaluated", but unevaluated string literals are a different kind of beast. I think that should be tracked on the argument level. We can either adjust:
> > ```
> > class StringArgument<string name, bit opt = 0> : Argument<name, opt>;
> > ```
> > so that it takes another bit for whether the string is unevaluated or not, or we could add a new subclass for `UnevaluatedStringArgument`. Then ClangAttrEmitter.cpp would look at this information when emitting the switch cases.
> This is the previous approach i forgot to fixup everywhere.
> My current approach is to always consider StringArgument unevaluated.
> I don't think it make sense to have both StringArgument and UnevaluatedStringArgument.
> Currently in all the places we accept StringArgument, we check it's a possibly parenthesized StringLiteral
> 
> If you want an evaluated string literal, an expression that produce a const char* or something should work
> My current approach is to always consider StringArgument unevaluated.
> I don't think it make sense to have both StringArgument and UnevaluatedStringArgument.

I think that's potentially a pretty significant change in behavior until we actually evaluate (ahahaha, puns!) all the vendor attributes using a `StringArgument`. Also, I thought you mentioned you planned to leave variadic string arguments as evaluated strings, so there would be a pretty surprising inconsistency to the behavior there. I would feel more comfortable not changing the behavior of attributes we've not validated are still correct when using unevaluated strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759