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

[Eugene.Zelenko]

clang-tidy/abseil/DurationDoubleConversionCheck.cpp

48 ↗

(On Diff #183945)

Please run Clang-format.

docs/ReleaseNotes.rst

85

Finds and fixes. Please use `` for language constructs. Same in documentation.

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

20 ↗

(On Diff #183945)

Unnecessary empty line.

27 ↗

(On Diff #183945)

Unnecessary empty line.

[hwright]

Address reviewer comments.

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

20 ↗

(On Diff #183945)

This is consistent with other documentation in this directory, such as abseil-faster-strsplit-delimiter.rst.

[Eugene.Zelenko]

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

28 ↗

(On Diff #184143)

Please use `` for language constructs. Same below.

[hwright]

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

28 ↗

(On Diff #184143)

Thanks; I thought I'd caught them all.

[MyDeveloperDay]

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

20 ↗

(On Diff #183945)

In your example abseil-faster-strsplit-delimiter.rst , The double blank line in the html doesn't give much delineation between the before and after code and the next example.

There probably isn't a convention per say (which is a shame), across the docs we do a mixture of different styles

https://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html
https://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html
https://clang.llvm.org/extra/clang-tidy/checks/google-objc-function-naming.html

But there is a desire by some of the regular clang-tidy reviewers to make the documentation consistent

It may not be ideal but the "Before/After" style, that is used in modernize-use-emplace, modernize-use-using,readability-braces-around-statements,readability-identifier-naming and readability-redundant-function-ptr-dereference does help a little.

I'm not saying looks better, but I've added a couple of examples of formatting the strsplit example for comparison, feel free to ignore.

[hwright]

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

20 ↗

(On Diff #183945)

I like those examples!

Would it be reasonable to update all of the abseil-duration-* documentation in a separate pass, after this change is submitted?

[MyDeveloperDay]

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

20 ↗

(On Diff #183945)

If it means the documentation gets better in the long run, I'm fine with that, its mainly a Nit on my part.

[hokein]

The code looks good, but I have a concern about the check name -- double seems a confusing word, see my comment.

clang-tidy/abseil/DurationDoubleConversionCheck.cpp

27 ↗

(On Diff #184146)

nit: s => S; might be inline the Scales?

51 ↗

(On Diff #184146)

The double word may cause confusion easily -- at the first glance, I thought that it means convert the absl::Duration to the double type, but it turned out not.. How about using abseil-duration-necessary-conversion?

[hwright]

Renamed to abseil-duration-unnecessary-conversion

[hwright]

@hokein Thanks for the suggestion on the name, I was looking for something a little less confusing.

[hokein]

LGTM

clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp

51

also avoid double word in the message.

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

18

and here.

26

and here.

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "DurationAdditionCheck.h"
 #include "DurationComparisonCheck.h"
 #include "DurationConversionCastCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
 #include "DurationSubtractionCheck.h"
+#include "DurationUnnecessaryConversionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     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");
+    CheckFactories.registerCheck<DurationUnnecessaryConversionCheck>(
+        "abseil-duration-unnecessary-conversion");
     CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
         "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<NoInternalDependenciesCheck>(
         "abseil-no-internal-dependencies");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
     CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
         "abseil-redundant-strcat-calls");
     CheckFactories.registerCheck<StrCatAppendCheck>(

clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationAdditionCheck.cpp
   DurationComparisonCheck.cpp
   DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp
+  DurationUnnecessaryConversionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 

clang-tidy/abseil/DurationUnnecessaryConversionCheck.h

+//===--- DurationUnnecessaryConversionCheck.h - clang-tidy ------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMEDOUBLECONVERSIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMEDOUBLECONVERSIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Finds and fixes cases where ``absl::Duration`` values are being converted
+/// to numeric types and back again.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-unnecessary-conversion.html
+class DurationUnnecessaryConversionCheck : public ClangTidyCheck {
+public:
+  DurationUnnecessaryConversionCheck(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_TIMEDOUBLECONVERSIONCHECK_H

clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp

+//===--- DurationUnnecessaryConversionCheck.cpp - clang-tidy
+//-----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DurationUnnecessaryConversionCheck.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 DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
+  for (const auto &Scale : {"Hours", "Minutes", "Seconds", "Milliseconds",
+                            "Microseconds", "Nanoseconds"}) {
+    std::string DurationFactory = (llvm::Twine("::absl::") + Scale).str();
+    std::string FloatConversion =
+        (llvm::Twine("::absl::ToDouble") + Scale).str();
+    std::string IntegerConversion =
+        (llvm::Twine("::absl::ToInt64") + Scale).str();
+
+    Finder->addMatcher(
+        callExpr(
+            callee(functionDecl(hasName(DurationFactory))),
+            hasArgument(0, callExpr(callee(functionDecl(hasAnyName(
+                                        FloatConversion, IntegerConversion))),
+                                    hasArgument(0, expr().bind("arg")))))
+            .bind("call"),
+        this);
+  }
+}
+
+void DurationUnnecessaryConversionCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *OuterCall = Result.Nodes.getNodeAs<Expr>("call");
+  const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+
+  if (!isNotInMacro(Result, OuterCall))
+    return;
+
+  diag(OuterCall->getBeginLoc(), "remove unnecessary absl::Duration conversions")
+      << FixItHint::CreateReplacement(
+             OuterCall->getSourceRange(),
+             tooling::fixit::getText(*Arg, *Result.Context));
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New :doc:`abseil-duration-addition
   domain.
 
 - 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.
 
+- New :doc:`abseil-duration-unnecessary-conversion
+  <clang-tidy/checks/abseil-duration-unnecessary-conversion>` check.
+
+  Finds and fixes cases where ``absl::Duration`` values are being converted to
+  numeric types and back again.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

+.. title:: clang-tidy - abseil-duration-unnecessary-conversion
+
+abseil-duration-unnecessary-conversion
+======================================
+
+Finds and fixes cases where ``absl::Duration`` values are being converted to
+numeric types and back again.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Conversion to double and back again
+  absl::Duration d1;
+  absl::Duration d2 = absl::Seconds(absl::ToDoubleSeconds(d1));
+
+  // Suggestion - Remove unnecessary conversions
+  absl::Duration d2 = d1;
+
+
+  // Original - Conversion to integer and back again
+  absl::Duration d1;
+  absl::Duration d2 = absl::Hours(absl::ToInt64Hours(d1));
+
+  // Suggestion - Remove unnecessary conversions
+  absl::Duration d2 = d1;
+
+Note: Converting to an integer and back to an ``absl::Duration`` might be a
+truncating operation if the value is not aligned to the scale of conversion.
+In the rare case where this is the intended result, callers should use
+``absl::Trunc`` to truncate explicitly.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-addition
    abseil-duration-comparison
    abseil-duration-conversion-cast
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-duration-subtraction
+   abseil-duration-unnecessary-conversion
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-str-cat-append
    abseil-string-find-startswith
    abseil-upgrade-duration-conversions
    android-cloexec-accept

test/clang-tidy/Inputs/absl/time/time.h

 // Mimic the implementation of absl::Duration
 namespace absl {
 
 using int64_t = long long int;
 
 class Duration {
 public:
   Duration &operator*=(int64_t r);
   Duration &operator*=(float r);
   Duration &operator*=(double r);
   template <typename T> Duration &operator*=(T r);
 
   Duration &operator/=(int64_t r);
   Duration &operator/=(float r);
   Duration &operator/=(double r);
   template <typename T> Duration &operator/=(T r);
+
+  Duration &operator+(Duration d);
 };
 
 template <typename T> Duration operator*(Duration lhs, T rhs);
 template <typename T> Duration operator*(T lhs, Duration rhs);
 template <typename T> Duration operator/(Duration lhs, T rhs);
 
 class Time{};
 

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

+// RUN: %check_clang_tidy %s abseil-duration-unnecessary-conversion %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  absl::Duration d1, d2;
+
+  // Floating point
+  d2 = absl::Hours(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Minutes(absl::ToDoubleMinutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Seconds(absl::ToDoubleSeconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Milliseconds(absl::ToDoubleMilliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Microseconds(absl::ToDoubleMicroseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Nanoseconds(absl::ToDoubleNanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+
+  // Integer point
+  d2 = absl::Hours(absl::ToInt64Hours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Minutes(absl::ToInt64Minutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Seconds(absl::ToInt64Seconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Milliseconds(absl::ToInt64Milliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Microseconds(absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Nanoseconds(absl::ToInt64Nanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+
+  // As macro argument
+#define PLUS_FIVE_S(x) x + absl::Seconds(5)
+  d2 = PLUS_FIVE_S(absl::Seconds(absl::ToInt64Seconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: PLUS_FIVE_S(d1)
+#undef PLUS_FIVE_S
+
+  // Split by macro: should not change
+#define TOSECONDS(x) absl::Seconds(x)
+  d2 = TOSECONDS(absl::ToInt64Seconds(d1));
+#undef TOSECONDS
+
+  // Don't change something inside a macro definition
+#define VALUE(x) absl::Hours(absl::ToInt64Hours(x));
+  d2 = VALUE(d1);
+#undef VALUE
+
+  // These should not match
+  d2 = absl::Seconds(absl::ToDoubleMilliseconds(d1));
+  d2 = absl::Seconds(4);
+  int i = absl::ToInt64Milliseconds(d1);
+}