Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -16,6 +16,7 @@ #include "DurationFactoryFloatCheck.h" #include "DurationFactoryScaleCheck.h" #include "DurationSubtractionCheck.h" +#include "DurationUnnecessaryConversionCheck.h" #include "FasterStrsplitDelimiterCheck.h" #include "NoInternalDependenciesCheck.h" #include "NoNamespaceCheck.h" @@ -45,6 +46,8 @@ "abseil-duration-factory-scale"); CheckFactories.registerCheck( "abseil-duration-subtraction"); + CheckFactories.registerCheck( + "abseil-duration-unnecessary-conversion"); CheckFactories.registerCheck( "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck( Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -10,6 +10,7 @@ DurationFactoryScaleCheck.cpp DurationRewriter.cpp DurationSubtractionCheck.cpp + DurationUnnecessaryConversionCheck.cpp FasterStrsplitDelimiterCheck.cpp NoInternalDependenciesCheck.cpp NoNamespaceCheck.cpp Index: clang-tidy/abseil/DurationUnnecessaryConversionCheck.h =================================================================== --- clang-tidy/abseil/DurationUnnecessaryConversionCheck.h +++ clang-tidy/abseil/DurationUnnecessaryConversionCheck.h @@ -0,0 +1,35 @@ +//===--- 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 Index: clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp =================================================================== --- clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp +++ clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp @@ -0,0 +1,58 @@ +//===--- 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("call"); + const auto *Arg = Result.Nodes.getNodeAs("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 Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -79,6 +79,12 @@ Checks for casts of ``absl::Duration`` conversion functions, and recommends the right conversion function instead. +- New :doc:`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 ` check. Index: docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst =================================================================== --- docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst +++ docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst @@ -0,0 +1,31 @@ +.. 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. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -11,6 +11,7 @@ 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 Index: test/clang-tidy/Inputs/absl/time/time.h =================================================================== --- test/clang-tidy/Inputs/absl/time/time.h +++ test/clang-tidy/Inputs/absl/time/time.h @@ -14,6 +14,8 @@ Duration &operator/=(float r); Duration &operator/=(double r); template Duration &operator/=(T r); + + Duration &operator+(Duration d); }; template Duration operator*(Duration lhs, T rhs); Index: test/clang-tidy/abseil-duration-unnecessary-conversion.cpp =================================================================== --- test/clang-tidy/abseil-duration-unnecessary-conversion.cpp +++ test/clang-tidy/abseil-duration-unnecessary-conversion.cpp @@ -0,0 +1,69 @@ +// 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); +}