steffenlarsen added a comment. I agree that reusing existing parser logic for this would be preferable, but as @aaron.ballman mentions `Parser::ParseSimpleExpressionList` does not give us the parameter pack expansion parsing we need. We could potentially unify it with the logic from https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059 but on the other hand there could be potential benefits to using `Parser::ParseExpressionList` instead.
The primary difference between the behavior of `Parser::ParseExpressionList` and what we are looking for is that it will allow initializer lists as arguments. Since most (if not all) attributes check the validity of their expression arguments, they would likely fail accordingly if they get an initializer list, likely specifying they expected an argument of type X. It may be a blessing in disguise to parse these arguments as well as we are more permissive w.r.t. attribute arguments. Please let me know if there is anything fundamentally wrong with this line of logic. One concern I have is that we currently run `Actions.CorrectDelayedTyposInExpr` on the expression right after parsing it, then we run pack expansion, whereas `Parser::ParseExpressionList` only runs the former action in failure case. I am unsure whether or not that might pose a problem. One problem is that if we only use `Parser::ParseExpressionList` to parse the arguments of the last argument (if it is variadic) then we get the odd behavior that the initializer lists will only be permitted in variadic arguments, which arguably is a little obscure. However, upon further investigation the only case that stops us from using this for all expression arguments are when the attribute has either `VariadicIdentifierArgument` or `VariadicParamOrParamIdxArgument` as the first argument. If we assume that these variadic expressions are limited to the last argument as well (as they are variadic) leaving them as the only argument of the attributes we are parsing here, then we can parse expressions as we did before if we are parsing one of these arguments or switch to using e.g. `Parser::ParseExpressionList` for all expressions of the attribute (even non-variadic) if not. Then we would just have to iterate over the produced expressions to make sure no parameter pack expansions occur before the last (variadic) argument of the parsed attribute. //Summary of suggested changes:// Split attribute parsing into 3: First case parsing a type (currently exists). Second case keeping the current logic for parsing identifiers and single assignment-expressions if the attribute has a single argument of type `VariadicIdentifierArgument` or `VariadicParamOrParamIdxArgument`. Third case, applicable if none of the former ran, parses all expression parameters in one using `Parser::ParseExpressionList` and then checks that parameter packs only occur in the last variadic argument (if it allows it). Side effect is it allows initializer lists in the third case, but attributes are likely to complain about unexpected expression types if these are passed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114439/new/ https://reviews.llvm.org/D114439 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
