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

[Jonas Toth via Phabricator via cfe-commits]

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
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<RedundantPpCheck>("readability-redundant-pp");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54349