[PATCH] Add a check to flag the usage of C-style casts (Google Style).

[Daniel Jasper]

Basically looks good, none of these other things are blockers.

================
Comment at: clang-tidy/google/CMakeLists.txt:4
@@ -3,2 +3,3 @@
 add_clang_library(clangTidyGoogleModule
+  CStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
----------------
Alexander Kornienko wrote:
> Daniel Jasper wrote:
> > I think the naming is not ideal. First, the "..Check" is mostly redundant information, maybe remove that, at least for new checks. Second, it would be good to verify what is being checked with c-style casts. So, maybe DontUseCStyleCasts.cpp or AvoidCStyleCasts.cpp.
> I see some value in using the "Check" suffix for checks: it makes class names nouns in a consistent way. I like it a bit more than using imperatives (e.g. UseOverride). The matter of the check can rarely be expressed using a single short imperative statement.
> 
> A few examples:
> 
>   * ArgumentCommentCheck checks that the correct argument name is used in the argument comment. I don't see a good alternative name in the imperative style (UseCorrectArgumentNameInArgumentComment?).
>   * one could name a check NameLocalVariablesCorrectly or LocalVariableNamingCheck. The latter seems a better alternative to me (though, it's a matter of taste).
>   * UseOverride check doesn't always ask you to "use override". It checks the correct usage of the virtual, override and final keywords on methods. In this case the name reflects the most frequent use case of the check (and is rather clear), but the name hints that this is the _only_ use case. An alternative name using noun notation could be more correct (but maybe a bit more bulky): VirtualOverrideFinalCheck or InheritanceSpecifierCheck.
> 
> That said, AvoidCStyleCasts (though, I would prefer AvoidCStyleCastsCheck) may be an appropriate name for the check in this patch.
I think there are pros/cons for many of these naming schemes. I'd mostly like us to settle on one and then be as consistent as possible.

Regarding your arguments:
- LocalVariableNamingCheck pretty precisely describes what it does, so that name is fine.
- ArgumentCommentCheck could intuitively also mean that it checks that arguments have comments. As we might eventually extend the check to require/add arguments for literals used as comments, this might be fine. Alternatively, we could rename it to something like ArgumentCommentNamingCheck.
- I agree that UseOverride might not be the best name. Generally, choosing the name based on the most frequent case does not seem like a bad idea, though. Maybe we should call it PreferOverrideCheck? This adds "..Check" and slightly weakens the hint that it is the only use case.
- I'd still prefer AvoidCStyleCastsCheck over CStyleCastsCheck, as CStyleCastsCheck does not at all say, what it does. It could also check various other things (spacing, redundant casts, ..).

================
Comment at: test/clang-tidy/c-style-casts.cpp:1
@@ +1,2 @@
+// RUN: clang-tidy -checks=-*,google-readability-casting %s -- | FileCheck %s
+
----------------
Alexander Kornienko wrote:
> Daniel Jasper wrote:
> > Maybe use check_clang_tidy_output.sh to decouple the check configuration.
> This script brings a bit of convenience, but it adds a requirement to use shell for tests, which limits the environments where the tests can run. However, the other script - check_clang_tidy_fix.sh - brings significant value, as it reduces a lot of shell code duplication from the RUN lines in the tests.
> 
> So I'd avoid using check_clang_tidy_output.sh. I'm not sure we actually need it, maybe just for consistency in cases where we have tests both for output and for fixes.
Well, I think we either need it or not. Consistency for cases where we have a fix seems like a bad distinction as we will hopefully have fixes for almost all checks (including this one).

We can fix this later (don't want to block this patch on it), but to me it seems like we should write the tests consistently (and fix this soon, before we have hundreds of tests). I don't care which way, my data-point would be that parameter changes to clang-tidy (like we have done with how the checks are configured) are significantly easier when clang-tidy is wrapped.

http://reviews.llvm.org/D4189