[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