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

Miklos Vajna via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Nov 10 12:23:34 PST 2018


vmiklos added a comment.

> 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?)

>   #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.

> Please keep this list sorted alphabetically.

Done.

> This name is not particularly descriptive. This seems to be more like
>  `CheckMacroRedundancy` or something like that?

Makes sense, done.

> This comment should be re-flowed to fit the column width.

Done.

> What constitutes "redundancy"? A bit more exposition here would be useful.

Hopefully "nested directives with the same condition" makes it easier to
understand the intention and current scope.


https://reviews.llvm.org/D54349





More information about the cfe-commits mailing list