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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to