[PATCH] D16310: new clang-tidy checker misc-long-cast

[Alexander Kornienko via cfe-commits]

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