[clang-tidy] Add a checker that suggests replacing short/long/long long with fixed-size types.

[alexfh]

clang-tidy/google/IntegerTypesCheck.cpp

84 ↗

(On Diff #13078)

Why not static const StringRef?

94 ↗

(On Diff #13078)

nit:
The "Emit a warning. " part is obvious, I'd remove it.

I'd swap the other two sentences and put each starting from its own line:

// We don't add a fix-it as changing the type can easily break code, 
// e.g. when a function requires a 'long' argument on all platforms.
// QualTypes are printed with implicit quotes.

clang-tidy/google/IntegerTypesCheck.h

20 ↗

(On Diff #13078)

Maybe:

... with u?intXX(_t)?

?

25 ↗

(On Diff #13078)

I'd use a bool flag instead of TypeSuffix or at least would add a comment describing the "_t" option for standard types.

30 ↗

(On Diff #13078)

Mark these fields "const"?

[bkramer]

Address review comments.

Closed by commit rL216726 (authored by d0k).

[sbenza]

clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp

45

The name should be more explicit.

clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp

20

Move this to ASTMatchers.h

clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp

4

This will fail on some platforms, right? "long" is not 64 bits everywhere (which is the point of this clang-tidy check).

[alexfh]

clang-tidy/google/IntegerTypesCheck.cpp

86 ↗

(On Diff #13082)

static const StringRef would save some strlen calls.

Also, when I was asking about StringRef, I had a thought that it could help avoiding std::strncmp. I was wrong, so StringRef doesn't make things any better. My bad. But it was a question, not a request to change it.

97 ↗

(On Diff #13082)

We should somehow hint the user, which header the new types are in.

clang-tidy/google/IntegerTypesCheck.h

19 ↗

(On Diff #13082)

It also finds unsigned versions of these, right?

26 ↗

(On Diff #13082)

I think, it makes sense noting that we need "_t" for standard types defined in <stdint.h>.

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   GoogleTidyModule.cpp
+  IntegerTypesCheck.cpp
   MemsetZeroLengthCheck.cpp
   NamedParameterCheck.cpp
   OverloadedUnaryAndCheck.cpp
   StringReferenceMemberCheck.cpp
   UnnamedNamespaceInHeaderCheck.cpp
   UsingNamespaceDirectiveCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/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 "AvoidCStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
+#include "IntegerTypesCheck.h"
 #include "MemsetZeroLengthCheck.h"
 #include "NamedParameterCheck.h"
 #include "OverloadedUnaryAndCheck.h"
 #include "StringReferenceMemberCheck.h"
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 
 using namespace clang::ast_matchers;
CheckFactories.addCheckFactory(
         new ClangTidyCheckFactory<build::UnnamedNamespaceInHeaderCheck>());
     CheckFactories.addCheckFactory(
         "google-build-using-namespace",
         new ClangTidyCheckFactory<build::UsingNamespaceDirectiveCheck>());
     CheckFactories.addCheckFactory(
         "google-explicit-constructor",
         new ClangTidyCheckFactory<ExplicitConstructorCheck>());
     CheckFactories.addCheckFactory(
+        "google-runtime-int",
+        new ClangTidyCheckFactory<runtime::IntegerTypesCheck>());
+    CheckFactories.addCheckFactory(
         "google-runtime-operator",
         new ClangTidyCheckFactory<runtime::OverloadedUnaryAndCheck>());
     CheckFactories.addCheckFactory(
         "google-runtime-member-string-references",
         new ClangTidyCheckFactory<runtime::StringReferenceMemberCheck>());
     CheckFactories.addCheckFactory(
         "google-runtime-memset",
         new ClangTidyCheckFactory<runtime::MemsetZeroLengthCheck>());

clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.h

+//===--- IntegerTypesCheck.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_INTEGERTYPESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_INTEGERTYPESCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace runtime {
+
+/// \brief Finds uses of short, long and long long and suggest replacing them
+/// with u?intXX(_t)?.
+/// Correspondig cpplint.py check: runtime/int.
+class IntegerTypesCheck : public ClangTidyCheck {
+public:
+  IntegerTypesCheck()
+      : UnsignedTypePrefix("uint"), SignedTypePrefix("int"),
+        AddUnderscoreT(false) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const StringRef UnsignedTypePrefix;
+  const StringRef SignedTypePrefix;
+  const bool AddUnderscoreT;
+};
+
+} // namespace runtime
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_INTEGERTYPESCHECK_H

clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp

+//===--- IntegerTypesCheck.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 "IntegerTypesCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/CharInfo.h"
+#include "clang/Basic/TargetInfo.h"
+
+namespace clang {
+
+namespace ast_matchers {
+const internal::VariadicDynCastAllOfMatcher<Decl, TypedefDecl> typedefDecl;
+} // namespace ast_matchers
+
+namespace tidy {
+namespace runtime {
+
+using namespace ast_matchers;
+
+void IntegerTypesCheck::registerMatchers(MatchFinder *Finder) {
+  // Find all TypeLocs.
+  Finder->addMatcher(typeLoc().bind("tl"), this);
+}
+
+void IntegerTypesCheck::check(const MatchFinder::MatchResult &Result) {
+  auto TL = *Result.Nodes.getNodeAs<TypeLoc>("tl");
+  SourceLocation Loc = TL.getLocStart();
+
+  if (Loc.isMacroID())
+    return;
+
+  // Look through qualification.
+  if (auto QualLoc = TL.getAs<QualifiedTypeLoc>())
+    TL = QualLoc.getUnqualifiedLoc();
+
+  auto BuiltinLoc = TL.getAs<BuiltinTypeLoc>();
+  if (!BuiltinLoc)
+    return;
+
+  bool IsSigned;
+  unsigned Width;
+  const TargetInfo &TargetInfo = Result.Context->getTargetInfo();
+
+  // Look for uses of short, long, long long and their unsigned versions.
+  switch (BuiltinLoc.getTypePtr()->getKind()) {
+  case BuiltinType::Short:
+    Width = TargetInfo.getShortWidth();
+    IsSigned = true;
+    break;
+  case BuiltinType::Long:
+    Width = TargetInfo.getLongWidth();
+    IsSigned = true;
+    break;
+  case BuiltinType::LongLong:
+    Width = TargetInfo.getLongLongWidth();
+    IsSigned = true;
+    break;
+  case BuiltinType::UShort:
+    Width = TargetInfo.getShortWidth();
+    IsSigned = false;
+    break;
+  case BuiltinType::ULong:
+    Width = TargetInfo.getLongWidth();
+    IsSigned = false;
+    break;
+  case BuiltinType::ULongLong:
+    Width = TargetInfo.getLongLongWidth();
+    IsSigned = false;
+    break;
+  default:
+    return;
+  }
+
+  // We allow "unsigned short port" as that's reasonably common and required by
+  // the sockets API.
+  const StringRef Port = "unsigned short port";
+  const char *Data = Result.SourceManager->getCharacterData(Loc);
+  if (!std::strncmp(Data, Port.data(), Port.size()) &&
+      !isIdentifierBody(Data[Port.size()]))
+    return;
+
+  std::string Replacement =
+      ((IsSigned ? SignedTypePrefix : UnsignedTypePrefix) + Twine(Width) +
+       (AddUnderscoreT ? "_t" : "")).str();
+
+  // We don't add a fix-it as changing the type can easily break code,
+  // e.g. when a function requires a 'long' argument on all platforms.
+  // QualTypes are printed with implicit quotes.
+  diag(Loc, "consider replacing %0 with '%1'") << BuiltinLoc.getType()
+                                               << Replacement;
+}
+
+} // namespace runtime
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp

+// RUN: clang-tidy -checks=-*,google-runtime-int %s -- -x c++ 2>&1 | FileCheck %s -implicit-check-not='{{warning:|error:}}'
+
+long a();
+// CHECK: [[@LINE-1]]:1: warning: consider replacing 'long' with 'int64'
+
+typedef unsigned long long uint64; // NOLINT
+
+long b(long = 1);
+// CHECK: [[@LINE-1]]:1: warning: consider replacing 'long' with 'int64'
+// CHECK: [[@LINE-2]]:8: warning: consider replacing 'long' with 'int64'
+
+template <typename T>
+void tmpl() {
+  T i;
+}
+
+short bar(const short, unsigned short) {
+// CHECK: [[@LINE-1]]:1: warning: consider replacing 'short' with 'int16'
+// CHECK: [[@LINE-2]]:17: warning: consider replacing 'short' with 'int16'
+// CHECK: [[@LINE-3]]:24: warning: consider replacing 'unsigned short' with 'uint16'
+  long double foo = 42;
+  uint64 qux = 42;
+  unsigned short port;
+
+  const unsigned short bar = 0;
+// CHECK: [[@LINE-1]]:9: warning: consider replacing 'unsigned short' with 'uint16'
+  long long *baar;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'long long' with 'int64'
+  const unsigned short &bara = bar;
+// CHECK: [[@LINE-1]]:9: warning: consider replacing 'unsigned short' with 'uint16'
+  long const long moo = 1;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'long long' with 'int64'
+  long volatile long wat = 42;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'long long' with 'int64'
+  unsigned long y;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'unsigned long' with 'uint64'
+  unsigned long long **const *tmp;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'unsigned long long' with 'uint64'
+  unsigned long long **const *&z = tmp;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'unsigned long long' with 'uint64'
+  unsigned short porthole;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'unsigned short' with 'uint16'
+
+#define l long
+  l x;
+
+  tmpl<short>();
+// CHECK: [[@LINE-1]]:8: warning: consider replacing 'short' with 'int16'
+}
+
+void qux() {
+  short port;
+// CHECK: [[@LINE-1]]:3: warning: consider replacing 'short' with 'int16'
+}