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

[alexfh]

Summary

Add a check to flag the usage of C-style casts, as per Google Style
Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Casting#Casting

[djasper]

clang-tidy/google/CMakeLists.txt

4

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.

clang-tidy/google/CStyleCastsCheck.h

22

What is this?

clang-tidy/google/GoogleTidyModule.cpp

25

Maybe order alphabetically by check name?

test/clang-tidy/c-style-casts.cpp

2

Maybe use check_clang_tidy_output.sh to decouple the check configuration.

4

Out of curiosity, why would there be a c-style cast here?

[alexfh]

clang-tidy/google/CMakeLists.txt

4

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.

clang-tidy/google/CStyleCastsCheck.h

22

This is the name of the corresponding check in cpplint.py. I've added a comment.

clang-tidy/google/GoogleTidyModule.cpp

25

Done.

test/clang-tidy/c-style-casts.cpp

2

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.

4

There's no c-style cast here. It's a utility function used below. The placement of the CHECK-NOT may be confusing though. I'll put a blank line here, so that it doesn't look like it relates to this function. We need the CHECK-NOT before the first CHECK as a generic measure to make the check more strict.

[alexfh]

Reordered registration of the checks. Added a newline after CHECK-NOT.

[djasper]

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

clang-tidy/google/CMakeLists.txt

4

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, ..).

test/clang-tidy/c-style-casts.cpp

2

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.

clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
+  CStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
   GoogleTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangTidy
   )

clang-tidy/google/CStyleCastsCheck.h

+//===--- CStyleCastsCheck.h - clang-tidy ------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_C_STYLE_CASTS_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_C_STYLE_CASTS_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \brief Finds usages of C-style casts.
+///
+/// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Casting#Casting
+/// Corresponding cpplint.py check name: 'readability/casting'.
+class CStyleCastsCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_C_STYLE_CASTS_CHECK_H

clang-tidy/google/CStyleCastsCheck.cpp

+//===--- CStyleCastsCheck.cpp - clang-tidy ----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CStyleCastsCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void CStyleCastsCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      cStyleCastExpr(
+          // Filter out (EnumType)IntegerLiteral construct, which is generated
+          // for non-type template arguments of enum types.
+          // FIXME: Remove this once this is fixed in the AST.
+          unless(allOf(hasDestinationType(hasDeclaration(enumDecl())),
+                       hasSourceExpression(integerLiteral())))).bind("cast"),
+      this);
+}
+
+void CStyleCastsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *CastExpr = Result.Nodes.getNodeAs<CStyleCastExpr>("cast");
+
+  // Ignore casts in macros for now.
+  if (CastExpr->getLocStart().isMacroID())
+    return;
+
+  // Casting to void is an idiomatic way to mute "unused variable" and similar
+  // warnings.
+  if (CastExpr->getTypeAsWritten()->isVoidType())
+    return;
+
+  diag(CastExpr->getLocStart(), "C-style casts are discouraged. Use "
+                                "static_cast/const_cast/reinterpret_cast "
+                                "instead.");
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.cpp - clang-tidy --------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "CStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 class GoogleModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "google-explicit-constructor",
         new ClangTidyCheckFactory<ExplicitConstructorCheck>());
+    CheckFactories.addCheckFactory(
+        "google-readability-casting",
+        new ClangTidyCheckFactory<readability::CStyleCastsCheck>());
   }
 };
 
 // Register the GoogleTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<GoogleModule> X("google-module",
                                                     "Adds Google lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the GoogleModule.
 volatile int GoogleModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

test/clang-tidy/c-style-casts.cpp

+// RUN: clang-tidy -checks=-*,google-readability-casting %s -- | FileCheck %s
+
+// CHECK-NOT: warning:
+
+bool g() { return false; }
+
+void f(int a, double b) {
+  int b1 = (int)b;
+  // CHECK: :[[@LINE-1]]:12: warning: C-style casts are discouraged. Use static_cast{{.*}}
+
+  // CHECK-NOT: warning:
+  int b2 = int(b);
+  int b3 = static_cast<double>(b);
+  int b4 = b;
+  double aa = a;
+  (void)b2;
+  return (void)g();
+}
+
+// CHECK-NOT: warning:
+enum E { E1 = 1 };
+template <E e>
+struct A { static const E ee = e; };
+struct B : public A<E1> {};