[clang-tidy] Add abseil-duration-conversion-cast check

Differential Revision: https://reviews.llvm.org/D56532

llvm-svn: 351473

clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp

@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DurationComparisonCheck.h"
+#include "DurationConversionCastCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -32,6 +33,8 @@ public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DurationComparisonCheck>(
         "abseil-duration-comparison");
+    CheckFactories.registerCheck<DurationConversionCastCheck>(
+        "abseil-duration-conversion-cast");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");
     CheckFactories.registerCheck<DurationFactoryFloatCheck>(

clang-tools-extra/clang-tidy/abseil/CMakeLists.txt

@@ -3,6 +3,7 @@ set(LLVM_LINK_COMPONENTS support)
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationComparisonCheck.cpp
+  DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp

clang-tools-extra/clang-tidy/abseil/DurationConversionCastCheck.cpp

@@ -0,0 +1,85 @@
+//===--- DurationConversionCastCheck.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 "DurationConversionCastCheck.h"
+#include "DurationRewriter.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void DurationConversionCastCheck::registerMatchers(MatchFinder *Finder) {
+  auto CallMatcher = ignoringImpCasts(callExpr(
+      callee(functionDecl(DurationConversionFunction()).bind("func_decl")),
+      hasArgument(0, expr().bind("arg"))));
+
+  Finder->addMatcher(
+      expr(anyOf(
+          cxxStaticCastExpr(hasSourceExpression(CallMatcher)).bind("cast_expr"),
+          cStyleCastExpr(hasSourceExpression(CallMatcher)).bind("cast_expr"),
+          cxxFunctionalCastExpr(hasSourceExpression(CallMatcher))
+              .bind("cast_expr"))),
+      this);
+}
+
+void DurationConversionCastCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCast =
+      Result.Nodes.getNodeAs<ExplicitCastExpr>("cast_expr");
+
+  if (!isNotInMacro(Result, MatchedCast))
+    return;
+
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("func_decl");
+  const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+  StringRef ConversionFuncName = FuncDecl->getName();
+
+  llvm::Optional<DurationScale> Scale = getScaleForInverse(ConversionFuncName);
+  if (!Scale)
+    return;
+
+  // Casting a double to an integer.
+  if (MatchedCast->getTypeAsWritten()->isIntegerType() &&
+      ConversionFuncName.contains("Double")) {
+    llvm::StringRef NewFuncName = getInverseForScale(*Scale).second;
+
+    diag(MatchedCast->getBeginLoc(),
+         "duration should be converted directly to an integer rather than "
+         "through a type cast")
+        << FixItHint::CreateReplacement(
+               MatchedCast->getSourceRange(),
+               (llvm::Twine(NewFuncName.substr(2)) + "(" +
+                tooling::fixit::getText(*Arg, *Result.Context) + ")")
+                   .str());
+  }
+
+  // Casting an integer to a double.
+  if (MatchedCast->getTypeAsWritten()->isRealFloatingType() &&
+      ConversionFuncName.contains("Int64")) {
+    llvm::StringRef NewFuncName = getInverseForScale(*Scale).first;
+
+    diag(MatchedCast->getBeginLoc(), "duration should be converted directly to "
+                                     "a floating-piont number rather than "
+                                     "through a type cast")
+        << FixItHint::CreateReplacement(
+               MatchedCast->getSourceRange(),
+               (llvm::Twine(NewFuncName.substr(2)) + "(" +
+                tooling::fixit::getText(*Arg, *Result.Context) + ")")
+                   .str());
+  }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/abseil/DurationConversionCastCheck.h

@@ -0,0 +1,36 @@
+//===--- DurationConversionCastCheck.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_ABSEIL_DURATIONCONVERSIONCASTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCONVERSIONCASTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Checks for casts of ``absl::Duration`` conversion functions, and recommends
+/// the right conversion function instead.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-conversion-cast.html
+class DurationConversionCastCheck : public ClangTidyCheck {
+public:
+  DurationConversionCastCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCONVERSIONCASTCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -67,7 +67,11 @@ The improvements are...
 Improvements to clang-tidy
 --------------------------
 
-- ...
+- New :doc:`abseil-duration-conversion-cast
+  <clang-tidy/checks/abseil-duration-conversion-cast>` check.
+
+  Checks for casts of ``absl::Duration`` conversion functions, and recommends
+  the right conversion function instead.
 
 Improvements to include-fixer
 -----------------------------

clang-tools-extra/docs/clang-tidy/checks/abseil-duration-conversion-cast.rst

@@ -0,0 +1,31 @@
+.. title:: clang-tidy - abseil-duration-conversion-cast
+
+abseil-duration-conversion-cast
+===============================
+
+Checks for casts of ``absl::Duration`` conversion functions, and recommends
+the right conversion function instead.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Cast from a double to an integer
+  absl::Duration d;
+  int i = static_cast<int>(absl::ToDoubleSeconds(d));
+
+  // Suggested - Use the integer conversion function directly.
+  int i = absl::ToInt64Seconds(d);
+
+
+  // Original - Cast from a double to an integer
+  absl::Duration d;
+  double x = static_cast<double>(absl::ToInt64Seconds(d));
+
+  // Suggested - Use the integer conversion function directly.
+  double x = absl::ToDoubleSeconds(d);
+
+
+Note: In the second example, the suggested fix could yield a different result,
+as the conversion to integer could truncate.  In practice, this is very rare,
+and you should use ``absl::Trunc`` to perform this operation explicitly instead.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -5,6 +5,7 @@ Clang-Tidy Checks
 
 .. toctree::
    abseil-duration-comparison
+   abseil-duration-conversion-cast
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale

clang-tools-extra/test/clang-tidy/abseil-duration-conversion-cast.cpp

@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s abseil-duration-conversion-cast %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  absl::Duration d1;
+  double x;
+  int i;
+
+  i = static_cast<int>(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  x = static_cast<float>(absl::ToInt64Hours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleHours(d1);
+  i = static_cast<int>(absl::ToDoubleMinutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Minutes(d1);
+  x = static_cast<float>(absl::ToInt64Minutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d1);
+  i = static_cast<int>(absl::ToDoubleSeconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Seconds(d1);
+  x = static_cast<float>(absl::ToInt64Seconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d1);
+  i = static_cast<int>(absl::ToDoubleMilliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Milliseconds(d1);
+  x = static_cast<float>(absl::ToInt64Milliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d1);
+  i = static_cast<int>(absl::ToDoubleMicroseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Microseconds(d1);
+  x = static_cast<float>(absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+  i = static_cast<int>(absl::ToDoubleNanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Nanoseconds(d1);
+  x = static_cast<float>(absl::ToInt64Nanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d1);
+
+  // Functional-style casts
+  i = int(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  x = float(absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+
+  // C-style casts
+  i = (int) absl::ToDoubleHours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  x = (float) absl::ToInt64Microseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+
+  // Type aliasing
+  typedef int FancyInt;
+  typedef float FancyFloat;
+
+  FancyInt j = static_cast<FancyInt>(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  FancyFloat k = static_cast<FancyFloat>(absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: duration should be converted directly to a floating-piont number rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+
+  // Macro handling
+  // We want to transform things in macro arguments
+#define EXTERNAL(x) (x) + 5
+  i = EXTERNAL(static_cast<int>(absl::ToDoubleSeconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: duration should be converted directly to an integer rather than through a type cast [abseil-duration-conversion-cast]
+  // CHECK-FIXES: EXTERNAL(absl::ToInt64Seconds(d1));
+#undef EXTERNAL
+
+  // We don't want to transform this which get split across macro boundaries
+#define SPLIT(x) static_cast<int>((x)) + 5
+  i = SPLIT(absl::ToDoubleSeconds(d1));
+#undef SPLIT
+
+  // We also don't want to transform things inside of a macro definition
+#define INTERNAL(x) static_cast<int>(absl::ToDoubleSeconds((x))) + 5
+  i = INTERNAL(d1);
+#undef INTERNAL
+
+  // These shouldn't be converted
+  i = static_cast<int>(absl::ToInt64Seconds(d1));
+  i = static_cast<float>(absl::ToDoubleSeconds(d1));
+}