[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54349#1294325, @vmiklos wrote:

> > What do you think about code like:
> > 
> >   #if FOO == 4
> >   #if FOO == 4
> >   #endif
> >   #endif
> >   
> >   #if defined(FOO)
> >   #if defined(FOO)
> >   #endif
> >   #endif
> >   
> >   #if !defined(FOO)
> >   #if !defined(FOO)
> >   #endif
> >   #endif
>
> I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO == 4' string (the condition) from the `If()` callback. So far I only see how to get the start location of the condition and the whole range till the end of endif. Do you have a code pointer how to do this, please? (Or OK if I leave this for a follow-up patch?)


I think it requires manual work. There's a FIXME in `PPCallbacks::If()` to pass a list/tree of tokens that would make implementing this reasonable. I'd say it's fine as a follow-up patch.

>>   #if defined(FOO)
>>   #if !defined(FOO)
>>   #endif
>>   #endif
>>   
>>   #if !defined(FOO)
>>   #if defined(FOO)
>>   #endif
>>   #endif
> 
> I expect handling these correctly is more complex -- I would prefer focusing on matching conditons in this patch, and get back to inverted conditions in a follow-up patch. Is that OK? If we handle inverted conditions, then handling `a && b` and `!a || !b` would make sense as well for example.

I agree that it's more complex, but that's why I was asking for it -- I don't think the design you have currently will extend for these sort of cases, and I was hoping to cover more utility with the check to hopefully shake out those forward-looking design considerations. As it stands, I feel like this check is a bit too simplistic to have much value, but if you covered some of these other cases, it would alleviate that worry for me.


https://reviews.llvm.org/D54349