[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

20 ↗

(On Diff #180988)

nit: double blank line

28 ↗

(On Diff #180988)

nit: double blank line

31 ↗

(On Diff #180988)

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

20 ↗

(On Diff #180988)

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

28 ↗

(On Diff #180988)

This is intentional (and consistent with other documentation).

[Eugene.Zelenko]

clang-tidy/abseil/DurationConversionCastCheck.cpp

41 ↗

(On Diff #181125)

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

46 ↗

(On Diff #181204)

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

52 ↗

(On Diff #181204)

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

57 ↗

(On Diff #181204)

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

10 ↗

(On Diff #181204)

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-tools-extra/trunk/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-tools-extra/trunk/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-tools-extra/trunk/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-tools-extra/trunk/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

clang-tools-extra/trunk/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...

clang-tools-extra/trunk/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.

clang-tools-extra/trunk/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

clang-tools-extra/trunk/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));
+}