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

[hwright]

Summary

This suggests simplifying expressions which are casting conversion functions, such as static_cast<int>(absl::ToDoubleSeconds(...))

[MyDeveloperDay]

nit's come from running running validate_check.py on the rst file from D55523: [clang-tidy] Linting .rst documentation

$ ../clang-tidy/validate_check.py --rst abseil-duration-conversion-cast.rst
Checking abseil-duration-conversion-cast.rst...
warning: line 6 maximize 80 characters by joining:'[Checks for casts of `absl::Duration` conversion functions, and recommends]' and '[the...]

warning: line 20 multiple blank lines
warning: line 28 multiple blank lines

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

21

nit: double blank line

29

nit: double blank line

32

nit: I believe that 'instead.' can come onto this line and still be <= 80 characters (which might actually be a bug in validate_check.py that it doesn't detect it)

[hwright]

Update documentation line wrapping.

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

21

This is intentional (and consistent with other examples in other tools).

29

This is intentional (and consistent with other documentation).

[Eugene.Zelenko]

clang-tidy/abseil/DurationConversionCastCheck.cpp

42

Please run Clang-format.

[hwright]

Run clang-format

[hwright]

Ping.

[aaron.ballman]

clang-tidy/abseil/DurationComparisonCheck.cpp

47–48 ↗

(On Diff #181204)

I think this change (and the related removal) should be landed separately as a NFC commit once this one lands, as it's logically separate from the new check.

clang-tidy/abseil/DurationConversionCastCheck.cpp

47

You can drop the llvm:: qualifiers here and elsewhere.

53

Add a full stop to the end of the comment (same below).

58

This diagnostic message does not tell the user what they did wrong with their code. How about duration should be converted directly to integer|floating-point rather than through a type cast or something along those lines?

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

11

Can you add an example like:

typedef int FancyInt;

FancyInt j = static_cast<FancyInt>(absl::ToDoubleHours(d1));

to make sure the cast is looking through types as expected? Please add a similar test for floating-point casts.

[hwright]

Address reviewer comments

clang-tidy/abseil/DurationComparisonCheck.cpp

47–48 ↗

(On Diff #181204)

I've gone ahead and done the change separately, which should clean this up.

[aaron.ballman]

LGTM!

clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.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 "DurationComparisonCheck.h"
+#include "DurationConversionCastCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
 #include "DurationSubtractionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 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>(
         "abseil-duration-factory-float");
     CheckFactories.registerCheck<DurationFactoryScaleCheck>(
         "abseil-duration-factory-scale");
     CheckFactories.registerCheck<DurationSubtractionCheck>(
         "abseil-duration-subtraction");

clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationComparisonCheck.cpp
+  DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

clang-tidy/abseil/DurationConversionCastCheck.h

+//===--- 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-tidy/abseil/DurationConversionCastCheck.cpp

+//===--- 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

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 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
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

+.. 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.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-comparison
+   abseil-duration-conversion-cast
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-duration-subtraction
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls

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

+// 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));
+}