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