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

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked an inline comment as done.

================
Comment at: clang-tidy/misc/LongCastCheck.cpp:43
@@ +42,3 @@
+
+static unsigned getMaxCalculationWidth(ASTContext &C, const Expr *E) {
+  E = E->IgnoreParenImpCasts();
----------------
LegalizeAdulthood wrote:
> Prefer anonymous namespace over `static` to scope visibility.
As I read the LLVM coding standards we prefer static for functions.

http://llvm.org/docs/CodingStandards.html#anonymous-namespaces



================
Comment at: clang-tidy/misc/LongCastCheck.cpp:97
@@ +96,3 @@
+
+  if (!CastType->isIntegerType() || !SubType->isIntegerType())
+    return;
----------------
LegalizeAdulthood wrote:
> Why don't you check for casting a `float` expression to a `double` or `long double`?
> 
> Isn't this the exact same issue?
> 
> If so, add a test case for casting a `float` expression to `double` and a test case for casting a `double` expression to a `long double`.
in theory yes.. but somehow that feels strange to me. yes there will possibly be loss of precision in some decimals, that is normal when using floating point numbers. if such loss of precision would be unwanted then float should be avoided to start with.

so I do agree in theory but I don't think I would feel good about adding such warnings.


================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:61
@@ -59,1 +60,3 @@
+    CheckFactories.registerCheck<LongCastCheck>(
+        "misc-long-cast");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D16310