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

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 9 13:31:11 PST 2018

JonasToth added a comment.

Hi vmiklos,

thank you for working on this patch!
I added a few comments.

Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:83
+    CheckFactories.registerCheck<RedundantPpCheck>("readability-redundant-pp");
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.

Comment at: clang-tidy/readability/RedundantPpCheck.cpp:28
+  void Ifdef(clang::SourceLocation aLoc, const clang::Token &rMacroNameTok,
+             const clang::MacroDefinition &rMacroDefinition) override;
you are in namespace `clang`, you can ellide `clang::`

Comment at: clang-tidy/readability/RedundantPpCheck.cpp:37
+  Preprocessor &PP;
+  std::vector<Entry> IfdefStack;
+  std::vector<Entry> IfndefStack;
Maybe `SmallVector<Entry, 4>`? Would be better for performance.

Comment at: clang-tidy/readability/RedundantPpCheck.cpp:48
+RedundantPPCallbacks::RedundantPPCallbacks(ClangTidyCheck &Check,
+                                           Preprocessor &PP)
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).

Comment at: clang-tidy/readability/RedundantPpCheck.cpp:52
+void RedundantPPCallbacks::Ifdef(
+    clang::SourceLocation Loc, const clang::Token &MacroNameTok,
The two functions are copied, please remove this duplicatoin and refactor it to a general parametrized function.

Comment at: docs/ReleaseNotes.rst:70
+- New :doc:`readability-redundant-pp
+  <clang-tidy/checks/readability-redundant-pp>` check.
Please order the checks alphabetically in the release notes, and one empty line at the end is enough.

Comment at: docs/clang-tidy/checks/readability-redundant-pp.rst:8
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition.
This needs more explanation, please add `.. code-block:: c++` sections for the cases that demonstrate the issue.

Comment at: test/clang-tidy/readability-redundant-pp-ifdef.cpp:6
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-pp]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
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.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list