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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:21
@@ +20,3 @@
+  Finder->addMatcher(
+      returnStmt(
+          has(cStyleCastExpr(has(binaryOperator(anyOf(hasOperatorName("+"),
----------------
Any reason to limit this to returnStmt, varDecl and assignment? This pattern can appear in any expression and lead to equally incorrect results.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:22
@@ +21,3 @@
+      returnStmt(
+          has(cStyleCastExpr(has(binaryOperator(anyOf(hasOperatorName("+"),
+                                                      hasOperatorName("*"),
----------------
Why only c-style casts? The problem applies to static_cast as well. Not sure how likely a reinterpret_cast is to appear in this situation, maybe it should be handled as well.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:23
@@ +22,3 @@
+          has(cStyleCastExpr(has(binaryOperator(anyOf(hasOperatorName("+"),
+                                                      hasOperatorName("*"),
+                                                      hasOperatorName("<<")))))
----------------
Why other operators are not considered here? Subtraction, for example, can suffer the same problem.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:66
@@ +65,3 @@
+        return LHSWidth + Bits.getExtValue();
+      else
+        // Unknown bitcount, assume there is truncation.
----------------
No `else` after `return`, please.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:71
@@ +70,3 @@
+  }
+
+  else if (const auto *Uop = dyn_cast<UnaryOperator>(E)) {
----------------
Please remove the extra line breaks.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:100
@@ +99,3 @@
+
+  ASTContext &C = *Result.Context;
+
----------------
Ctx or Context, please.

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:61
@@ -59,1 +60,3 @@
+    CheckFactories.registerCheck<LongCastCheck>(
+        "misc-long-cast");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
----------------
danielmarjamaki wrote:
> LegalizeAdulthood wrote:
> > The documentation describes this check as one that looks for a cast to a "bigger type", but the name of the check implies that it only works for casts to `long`.
> > 
> > The name of the check should be made more generic to reflect reality.
> > 
> > Perhaps `misc-redundant-cast-to-larger-type` or `misc-redundant-bigger-type-cast`?
> Yes I agree.. will fix..
> 
> I used long because that is the typical case that I envision.
How about misc-misplaced-widening-cast?

================
Comment at: docs/clang-tidy/checks/misc-long-cast.rst:11
@@ +10,3 @@
+
+Example code::
+
----------------
LegalizeAdulthood wrote:
> Please add an example for another type other than `long`, such as casting a `float` expression to a `double`.
Is the use of two colons intended?

================
Comment at: test/clang-tidy/misc-long-cast.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-long-cast %t
+
----------------
Please add tests with templates: casting to or from a dependent type shouldn't trigger the warning.


Repository:
  rL LLVM

http://reviews.llvm.org/D16310