[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
Miklos Vajna via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 9 16:12:58 PST 2018
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::`
> Maybe `SmallVector<Entry, 4>`? Would be better for performance.
> 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).
> The two functions are copied, please remove this duplicatoin and refactor it
> to a general parametrized function.
> Please order the checks alphabetically in the release notes, and one empty
> line at the end is enough.
> This needs more explanation, please add `.. code-block:: c++` sections for
> the cases that demonstrate the issue.
> 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
> `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.)
More information about the cfe-commits