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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
