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

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

> I think that name is not very descriptive for the user of clang-tidy. `pp`
>  should be `preprocessor` or some other constellation that makes it very clear
>  its about the preprocessor.

Done, renamed to readability-redundant-preprocessor.

> you are in namespace `clang`, you can ellide `clang::`

Done.

> Maybe `SmallVector<Entry, 4>`? Would be better for performance.

Done.

> I think it would be better to have these methods defined inline, as the
>  splitting does not achieve its original goal (declaration in header,
>  definition in impl).

Done.

> The two functions are copied, please remove this duplicatoin and refactor it
>  to a general parametrized function.

Done.

> Please order the checks alphabetically in the release notes, and one empty
>  line at the end is enough.

Done.

> This needs more explanation, please add `.. code-block:: c++` sections for
>  the cases that demonstrate the issue.

Done.

> Please add a test where the redundancy comes from including other headers. If
>  the headers are nested this case might still occur, but its not safe to
>  diagnose/remove these checks as other include-places might not have the same
>  constellation.
> 
> `ifdef` and `ifndef` are used for the include-guards and therefore
>  particularly necessary to test.

Done. I've added a test to make sure we don't warn in headers, the code for
this was already there, just had no coverage. (Exactly for the reason you
mention, the possibility of include-guards generating false positives.)


https://reviews.llvm.org/D54349