[clang] [clang] Update argument checking tablegen code to use a 'full' name (PR #99993)
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 24 06:53:36 PDT 2024
AaronBallman wrote:
> > Does this impact anything user-facing? e.g., should there be an additional test somewhere in clang/test/Sema/ for this change?
>
> I don't think there is any user-visible issue with the one attribute that uses this.
Thanks!
> At least how the Parse code is written currently. The attribute has three EnumArguments followed by an IntArgument. The call to attributeHasStrictIdentifierArgAtIndex is only done if the token is an identifier and if the IntArgument is an identifier that same thing happens anyway.
>
> Looking at this code now, I am not convinced the call to attributeHasStrictIdentifierArgAtIndex does anything useful.
>
> ```
> if (Tok.is(tok::identifier) && attributeHasStrictIdentifierArgAtIndex(
> *AttrName, ArgExprs.size())) {
> ArgExprs.push_back(ParseIdentifierLoc());
> continue;
> }
>
> ExprResult ArgExpr;
> if (Tok.is(tok::identifier)) {
> ArgExprs.push_back(ParseIdentifierLoc());
> } else {
> ```
>
> So I think we can remove the whole function attributeHasStrictIdentifierArgAtIndex and the related tablegen. No tests fail without it.
>
> The function attributeHasStrictIdentifierArgs still seems necessary but not the AtIndex.
>
> What do you think?
That code was added very recently by https://github.com/llvm/llvm-project/pull/94056 and seem to be specific to the `ptrauth_vtable_pointer` attribute. Perhaps we're lacking test coverage if nothing breaks without that code?
CC @ojhunt @ahmedbougacha
https://github.com/llvm/llvm-project/pull/99993
More information about the cfe-commits
mailing list