alexfh added inline comments. ================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:20 @@ +19,3 @@ +void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) { + auto Calc = stmt(anyOf(binaryOperator(anyOf( + hasOperatorName("+"), hasOperatorName("-"), ---------------- It makes sense to use the closest common parent, which is `expr` here, not `stmt`.
================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:27 @@ +26,3 @@ + auto Cast = + stmt(anyOf(cStyleCastExpr(has(Calc)), cxxStaticCastExpr(has(Calc)), + cxxReinterpretCastExpr(has(Calc)))) ---------------- You can use `explicitCastExpr` instead of `stmt` and restructure the matcher to reduce code duplication: explicitCastExpr(anyOf(cStyleCastExpr(), cxxStaticCastExpr(), cxxReinterpretCastExpr()), has(Calc)) ================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:31 @@ +30,3 @@ + + Finder->addMatcher(returnStmt(has(Cast)), this); + Finder->addMatcher(varDecl(has(Cast)), this); ---------------- FYI, these matchers can be combined using `anyOf`. Not sure whether this will be better (readability-wise and performance-wise) or not. ================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:78 @@ +77,3 @@ + const auto *Cast = Result.Nodes.getNodeAs<Expr>("cast"); + if (!Cast || Cast->getLocStart().isMacroID()) + return; ---------------- IIUC, both "cast" and "calc" are bound in non-optional branches of the matcher, so `Cast` and `Calc` should never be `nullptr`. Please move the `!Cast` and `!Calc` checks from `if`s to asserts. ================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:88 @@ +87,3 @@ + + if (!CastType->isIntegerType() || !CalcType->isIntegerType()) + return; ---------------- You should be able to check this in the matcher (using `hasType(isInteger())` for the `expr` matcher and a bit more specific `hasDestinationType(isInteger())` for `explicitCastExpr`). ================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.h:16 @@ +15,3 @@ +namespace clang { +namespace tidy { + ---------------- Please add `namespace misc {`. BTW, do you know the `add_new_check.py` script that uses the currently recommended template? ================ Comment at: clang-tidy/misc/MisplacedWideningCastCheck.h:21 @@ +20,3 @@ +/// of precision then the cast is misplaced, and there can be loss of +/// precision. Otherwise such cast is ineffective. +class MisplacedWideningCastCheck : public ClangTidyCheck { ---------------- Please add a link to the user docs: // For the user-facing documentation see: // http://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-widening-cast.html (and next time use `add_new_check.py` ;) ================ Comment at: docs/clang-tidy/checks/misc-misplaced-widening-cast.rst:1 @@ +1,2 @@ +.. title:: clang-tidy - misc-misplaced-widening-cast + ---------------- If you wonder how to test the RST you write: 1. when running cmake, specify `-DCLANG_TOOLS_EXTRA_INCLUDE_DOCS=ON` (you can also turn on `LLVM_INCLUDE_DOCS` and `CLANG_INCLUDE_DOCS`, if you wish, but this shouldn't be necessary) 2. `make/ninja docs-clang-tools-html`, the results should be generated in "<your-build-directory>/tools/clang/tools/extra/docs/html" ================ Comment at: docs/clang-tidy/checks/misc-misplaced-widening-cast.rst:4 @@ +3,3 @@ +misc-misplaced-widening-cast +================================== + ---------------- nit: Please make the underline the same length as the previous line. ================ Comment at: test/clang-tidy/misc-misplaced-widening-cast.cpp:31 @@ +30,3 @@ + long l; + // the result is a 9 bit value, there is no truncation in the implicit cast + l = (long)(a + 15); ---------------- Please use proper punctuation and capitalization in the comments. http://reviews.llvm.org/D16310 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits