[clang-tools-extra] r348161 - [clang-tidy] Add the abseil-duration-comparison check
Jonas Toth via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 3 10:35:56 PST 2018
Author: jonastoth
Date: Mon Dec 3 10:35:56 2018
New Revision: 348161
URL: http://llvm.org/viewvc/llvm-project?rev=348161&view=rev
Log:
[clang-tidy] Add the abseil-duration-comparison check
Summary:
This check finds instances where Duration values are being converted to a numeric value in a comparison expression, and suggests that the conversion happen on the other side of the expression to a Duration. See documentation for examples.
This also shuffles some code around so that the new check may perform in sone step simplifications also caught by other checks.
Patch by hwright.
Reviewers: aaron.ballman, JonasToth, alexfh, hokein
Reviewed By: JonasToth
Subscribers: sammccall, Eugene.Zelenko, xazax.hun, cfe-commits, mgorny
Tags: #clang-tools-extra
Differential Revision: https://reviews.llvm.org/D54737
Added:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
Modified: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp?rev=348161&r1=348160&r2=348161&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp Mon Dec 3 10:35:56 2018
@@ -10,6 +10,7 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "DurationComparisonCheck.h"
#include "DurationDivisionCheck.h"
#include "DurationFactoryFloatCheck.h"
#include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@ namespace abseil {
class AbseilModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<DurationComparisonCheck>(
+ "abseil-duration-comparison");
CheckFactories.registerCheck<DurationDivisionCheck>(
"abseil-duration-division");
CheckFactories.registerCheck<DurationFactoryFloatCheck>(
Modified: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt?rev=348161&r1=348160&r2=348161&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt Mon Dec 3 10:35:56 2018
@@ -2,9 +2,11 @@ set(LLVM_LINK_COMPONENTS support)
add_clang_library(clangTidyAbseilModule
AbseilTidyModule.cpp
+ DurationComparisonCheck.cpp
DurationDivisionCheck.cpp
DurationFactoryFloatCheck.cpp
DurationFactoryScaleCheck.cpp
+ DurationRewriter.cpp
FasterStrsplitDelimiterCheck.cpp
NoInternalDependenciesCheck.cpp
NoNamespaceCheck.cpp
Added: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=348161&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Mon Dec 3 10:35:56 2018
@@ -0,0 +1,164 @@
+//===--- DurationComparisonCheck.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 "DurationComparisonCheck.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 {
+
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+ static const llvm::DenseMap<llvm::StringRef, DurationScale> ScaleMap(
+ {{"ToDoubleHours", DurationScale::Hours},
+ {"ToInt64Hours", DurationScale::Hours},
+ {"ToDoubleMinutes", DurationScale::Minutes},
+ {"ToInt64Minutes", DurationScale::Minutes},
+ {"ToDoubleSeconds", DurationScale::Seconds},
+ {"ToInt64Seconds", DurationScale::Seconds},
+ {"ToDoubleMilliseconds", DurationScale::Milliseconds},
+ {"ToInt64Milliseconds", DurationScale::Milliseconds},
+ {"ToDoubleMicroseconds", DurationScale::Microseconds},
+ {"ToInt64Microseconds", DurationScale::Microseconds},
+ {"ToDoubleNanoseconds", DurationScale::Nanoseconds},
+ {"ToInt64Nanoseconds", DurationScale::Nanoseconds}});
+
+ auto ScaleIter = ScaleMap.find(std::string(Name));
+ if (ScaleIter == ScaleMap.end())
+ return llvm::None;
+
+ return ScaleIter->second;
+}
+
+/// Given a `Scale` return the inverse functions for it.
+static const std::pair<llvm::StringRef, llvm::StringRef> &
+getInverseForScale(DurationScale Scale) {
+ static const std::unordered_map<DurationScale,
+ std::pair<llvm::StringRef, llvm::StringRef>>
+ InverseMap(
+ {{DurationScale::Hours,
+ std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")},
+ {DurationScale::Minutes, std::make_pair("::absl::ToDoubleMinutes",
+ "::absl::ToInt64Minutes")},
+ {DurationScale::Seconds, std::make_pair("::absl::ToDoubleSeconds",
+ "::absl::ToInt64Seconds")},
+ {DurationScale::Milliseconds,
+ std::make_pair("::absl::ToDoubleMilliseconds",
+ "::absl::ToInt64Milliseconds")},
+ {DurationScale::Microseconds,
+ std::make_pair("::absl::ToDoubleMicroseconds",
+ "::absl::ToInt64Microseconds")},
+ {DurationScale::Nanoseconds,
+ std::make_pair("::absl::ToDoubleNanoseconds",
+ "::absl::ToInt64Nanoseconds")}});
+
+ // We know our map contains all the Scale values, so we can skip the
+ // nonexistence check.
+ auto InverseIter = InverseMap.find(Scale);
+ assert(InverseIter != InverseMap.end() && "Unexpected scale found");
+ return InverseIter->second;
+}
+
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+ DurationScale Scale, const Expr &Node) {
+ const std::pair<std::string, std::string> &InverseFunctions =
+ getInverseForScale(Scale);
+ if (const Expr *MaybeCallArg = selectFirst<const Expr>(
+ "e", match(callExpr(callee(functionDecl(
+ hasAnyName(InverseFunctions.first.c_str(),
+ InverseFunctions.second.c_str()))),
+ hasArgument(0, expr().bind("e"))),
+ Node, *Result.Context))) {
+ return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+ }
+
+ return llvm::None;
+}
+
+/// Assuming `Node` has type `double` or `int` representing a time interval of
+/// `Scale`, return the expression to make it a suitable `Duration`.
+static llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+ const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+ const Expr *Node) {
+ const Expr &RootNode = *Node->IgnoreParenImpCasts();
+
+ if (RootNode.getBeginLoc().isMacroID())
+ return llvm::None;
+
+ // First check to see if we can undo a complimentary function call.
+ if (llvm::Optional<std::string> MaybeRewrite =
+ maybeRewriteInverseDurationCall(Result, Scale, RootNode))
+ return *MaybeRewrite;
+
+ if (IsLiteralZero(Result, RootNode))
+ return std::string("absl::ZeroDuration()");
+
+ return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
+ simplifyDurationFactoryArg(Result, RootNode) + ")")
+ .str();
+}
+
+void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
+ auto Matcher =
+ binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+ hasOperatorName("=="), hasOperatorName("<="),
+ hasOperatorName("<")),
+ hasEitherOperand(ignoringImpCasts(callExpr(
+ callee(functionDecl(DurationConversionFunction())
+ .bind("function_decl"))))))
+ .bind("binop");
+
+ Finder->addMatcher(Matcher, this);
+}
+
+void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+
+ // Don't try to replace things inside of macro definitions.
+ if (Binop->getExprLoc().isMacroID())
+ return;
+
+ llvm::Optional<DurationScale> Scale = getScaleForInverse(
+ Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
+ if (!Scale)
+ return;
+
+ // In most cases, we'll only need to rewrite one of the sides, but we also
+ // want to handle the case of rewriting both sides. This is much simpler if
+ // we unconditionally try and rewrite both, and let the rewriter determine
+ // if nothing needs to be done.
+ llvm::Optional<std::string> LhsReplacement =
+ rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
+ llvm::Optional<std::string> RhsReplacement =
+ rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
+
+ if (!(LhsReplacement && RhsReplacement))
+ return;
+
+ diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
+ << FixItHint::CreateReplacement(Binop->getSourceRange(),
+ (llvm::Twine(*LhsReplacement) + " " +
+ Binop->getOpcodeStr() + " " +
+ *RhsReplacement)
+ .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Added: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h?rev=348161&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h Mon Dec 3 10:35:56 2018
@@ -0,0 +1,36 @@
+//===--- DurationComparisonCheck.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_DURATIONCOMPARISONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Prefer comparison in the absl::Duration domain instead of the numeric
+/// domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-comparison.html
+class DurationComparisonCheck : public ClangTidyCheck {
+public:
+ DurationComparisonCheck(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_DURATIONCOMPARISONCHECK_H
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp?rev=348161&r1=348160&r2=348161&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp Mon Dec 3 10:35:56 2018
@@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//
#include "DurationFactoryFloatCheck.h"
+#include "DurationRewriter.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Tooling/FixIt.h"
@@ -18,19 +19,6 @@ namespace clang {
namespace tidy {
namespace abseil {
-// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
-static llvm::Optional<llvm::APSInt>
-truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
- double Value = FloatLiteral.getValueAsApproximateDouble();
- if (std::fmod(Value, 1) == 0) {
- if (Value >= static_cast<double>(1u << 31))
- return llvm::None;
-
- return llvm::APSInt::get(static_cast<int64_t>(Value));
- }
- return llvm::None;
-}
-
// Returns `true` if `Range` is inside a macro definition.
static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
SourceRange Range) {
@@ -42,21 +30,14 @@ static bool InsideMacroDefinition(const
void DurationFactoryFloatCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
- callExpr(
- callee(functionDecl(hasAnyName(
- "absl::Nanoseconds", "absl::Microseconds", "absl::Milliseconds",
- "absl::Seconds", "absl::Minutes", "absl::Hours"))),
- hasArgument(0,
- anyOf(cxxStaticCastExpr(
- hasDestinationType(realFloatingPointType()),
- hasSourceExpression(expr().bind("cast_arg"))),
- cStyleCastExpr(
- hasDestinationType(realFloatingPointType()),
- hasSourceExpression(expr().bind("cast_arg"))),
- cxxFunctionalCastExpr(
- hasDestinationType(realFloatingPointType()),
- hasSourceExpression(expr().bind("cast_arg"))),
- floatLiteral().bind("float_literal"))))
+ callExpr(callee(functionDecl(DurationFactoryFunction())),
+ hasArgument(0, anyOf(cxxStaticCastExpr(hasDestinationType(
+ realFloatingPointType())),
+ cStyleCastExpr(hasDestinationType(
+ realFloatingPointType())),
+ cxxFunctionalCastExpr(hasDestinationType(
+ realFloatingPointType())),
+ floatLiteral())))
.bind("call"),
this);
}
@@ -73,31 +54,16 @@ void DurationFactoryFloatCheck::check(co
if (Arg->getBeginLoc().isMacroID())
return;
- // Check for casts to `float` or `double`.
- if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) {
+ llvm::Optional<std::string> SimpleArg = stripFloatCast(Result, *Arg);
+ if (!SimpleArg)
+ SimpleArg = stripFloatLiteralFraction(Result, *Arg);
+
+ if (SimpleArg) {
diag(MatchedCall->getBeginLoc(),
(llvm::Twine("use the integer version of absl::") +
MatchedCall->getDirectCallee()->getName())
.str())
- << FixItHint::CreateReplacement(
- Arg->getSourceRange(),
- tooling::fixit::getText(*MaybeCastArg, *Result.Context));
- return;
- }
-
- // Check for floats without fractional components.
- if (const auto *LitFloat =
- Result.Nodes.getNodeAs<FloatingLiteral>("float_literal")) {
- // Attempt to simplify a `Duration` factory call with a literal argument.
- if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat)) {
- diag(MatchedCall->getBeginLoc(),
- (llvm::Twine("use the integer version of absl::") +
- MatchedCall->getDirectCallee()->getName())
- .str())
- << FixItHint::CreateReplacement(LitFloat->getSourceRange(),
- IntValue->toString(/*radix=*/10));
- return;
- }
+ << FixItHint::CreateReplacement(Arg->getSourceRange(), *SimpleArg);
}
}
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp?rev=348161&r1=348160&r2=348161&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp Mon Dec 3 10:35:56 2018
@@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//
#include "DurationFactoryScaleCheck.h"
+#include "DurationRewriter.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Tooling/FixIt.h"
@@ -18,20 +19,6 @@ namespace clang {
namespace tidy {
namespace abseil {
-namespace {
-
-// Potential scales of our inputs.
-enum class DurationScale {
- Hours,
- Minutes,
- Seconds,
- Milliseconds,
- Microseconds,
- Nanoseconds,
-};
-
-} // namespace
-
// Given the name of a duration factory function, return the appropriate
// `DurationScale` for that factory. If no factory can be found for
// `FactoryName`, return `None`.
@@ -129,39 +116,14 @@ static llvm::Optional<DurationScale> Get
return llvm::None;
}
-// Given a `Scale`, return the appropriate factory function call for
-// constructing a `Duration` for that scale.
-static llvm::StringRef GetFactoryForScale(DurationScale Scale) {
- switch (Scale) {
- case DurationScale::Hours:
- return "absl::Hours";
- case DurationScale::Minutes:
- return "absl::Minutes";
- case DurationScale::Seconds:
- return "absl::Seconds";
- case DurationScale::Milliseconds:
- return "absl::Milliseconds";
- case DurationScale::Microseconds:
- return "absl::Microseconds";
- case DurationScale::Nanoseconds:
- return "absl::Nanoseconds";
- }
- llvm_unreachable("unknown scaling factor");
-}
-
void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
callExpr(
- callee(functionDecl(
- hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
- "::absl::Milliseconds", "::absl::Seconds",
- "::absl::Minutes", "::absl::Hours"))
- .bind("call_decl")),
+ callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
hasArgument(
0,
ignoringImpCasts(anyOf(
- integerLiteral(equals(0)).bind("zero"),
- floatLiteral(equals(0.0)).bind("zero"),
+ integerLiteral(equals(0)), floatLiteral(equals(0.0)),
binaryOperator(hasOperatorName("*"),
hasEitherOperand(ignoringImpCasts(
anyOf(integerLiteral(), floatLiteral()))))
@@ -185,7 +147,7 @@ void DurationFactoryScaleCheck::check(co
return;
// We first handle the cases of literal zero (both float and integer).
- if (Result.Nodes.getNodeAs<Stmt>("zero")) {
+ if (IsLiteralZero(Result, *Arg)) {
diag(Call->getBeginLoc(),
"use ZeroDuration() for zero-length time intervals")
<< FixItHint::CreateReplacement(Call->getSourceRange(),
@@ -244,7 +206,7 @@ void DurationFactoryScaleCheck::check(co
diag(Call->getBeginLoc(), "internal duration scaling can be removed")
<< FixItHint::CreateReplacement(
Call->getSourceRange(),
- (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
+ (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
tooling::fixit::getText(*Remainder, *Result.Context) + ")")
.str());
}
@@ -257,7 +219,7 @@ void DurationFactoryScaleCheck::check(co
diag(Call->getBeginLoc(), "internal duration scaling can be removed")
<< FixItHint::CreateReplacement(
Call->getSourceRange(),
- (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
+ (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
tooling::fixit::getText(*Remainder, *Result.Context) + ")")
.str());
}
Added: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp?rev=348161&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp Mon Dec 3 10:35:56 2018
@@ -0,0 +1,109 @@
+//===--- DurationRewriter.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 "DurationRewriter.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
+static llvm::Optional<llvm::APSInt>
+truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
+ double Value = FloatLiteral.getValueAsApproximateDouble();
+ if (std::fmod(Value, 1) == 0) {
+ if (Value >= static_cast<double>(1u << 31))
+ return llvm::None;
+
+ return llvm::APSInt::get(static_cast<int64_t>(Value));
+ }
+ return llvm::None;
+}
+
+/// Returns the factory function name for a given `Scale`.
+llvm::StringRef getFactoryForScale(DurationScale Scale) {
+ switch (Scale) {
+ case DurationScale::Hours:
+ return "absl::Hours";
+ case DurationScale::Minutes:
+ return "absl::Minutes";
+ case DurationScale::Seconds:
+ return "absl::Seconds";
+ case DurationScale::Milliseconds:
+ return "absl::Milliseconds";
+ case DurationScale::Microseconds:
+ return "absl::Microseconds";
+ case DurationScale::Nanoseconds:
+ return "absl::Nanoseconds";
+ }
+ llvm_unreachable("unknown scaling factor");
+}
+
+/// Returns `true` if `Node` is a value which evaluates to a literal `0`.
+bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) {
+ return selectFirst<const clang::Expr>(
+ "val",
+ match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)),
+ floatLiteral(equals(0.0)))))
+ .bind("val"),
+ Node, *Result.Context)) != nullptr;
+}
+
+llvm::Optional<std::string>
+stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &Node) {
+ if (const Expr *MaybeCastArg = selectFirst<const Expr>(
+ "cast_arg",
+ match(expr(anyOf(cxxStaticCastExpr(
+ hasDestinationType(realFloatingPointType()),
+ hasSourceExpression(expr().bind("cast_arg"))),
+ cStyleCastExpr(
+ hasDestinationType(realFloatingPointType()),
+ hasSourceExpression(expr().bind("cast_arg"))),
+ cxxFunctionalCastExpr(
+ hasDestinationType(realFloatingPointType()),
+ hasSourceExpression(expr().bind("cast_arg"))))),
+ Node, *Result.Context)))
+ return tooling::fixit::getText(*MaybeCastArg, *Result.Context).str();
+
+ return llvm::None;
+}
+
+llvm::Optional<std::string>
+stripFloatLiteralFraction(const MatchFinder::MatchResult &Result,
+ const Expr &Node) {
+ if (const auto *LitFloat = llvm::dyn_cast<FloatingLiteral>(&Node))
+ // Attempt to simplify a `Duration` factory call with a literal argument.
+ if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat))
+ return IntValue->toString(/*radix=*/10);
+
+ return llvm::None;
+}
+
+std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result,
+ const Expr &Node) {
+ // Check for an explicit cast to `float` or `double`.
+ if (llvm::Optional<std::string> MaybeArg = stripFloatCast(Result, Node))
+ return *MaybeArg;
+
+ // Check for floats without fractional components.
+ if (llvm::Optional<std::string> MaybeArg =
+ stripFloatLiteralFraction(Result, Node))
+ return *MaybeArg;
+
+ // We couldn't simplify any further, so return the argument text.
+ return tooling::fixit::getText(Node, *Result.Context).str();
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Added: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h?rev=348161&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h Mon Dec 3 10:35:56 2018
@@ -0,0 +1,86 @@
+//===--- DurationRewriter.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_DURATIONREWRITER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include <cinttypes>
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Duration factory and conversion scales
+enum class DurationScale : std::int8_t {
+ Hours,
+ Minutes,
+ Seconds,
+ Milliseconds,
+ Microseconds,
+ Nanoseconds,
+};
+
+/// Given a `Scale`, return the appropriate factory function call for
+/// constructing a `Duration` for that scale.
+llvm::StringRef getFactoryForScale(DurationScale Scale);
+
+// Determine if `Node` represents a literal floating point or integral zero.
+bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &Node);
+
+/// Possibly strip a floating point cast expression.
+///
+/// If `Node` represents an explicit cast to a floating point type, return
+/// the textual context of the cast argument, otherwise `None`.
+llvm::Optional<std::string>
+stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &Node);
+
+/// Possibly remove the fractional part of a floating point literal.
+///
+/// If `Node` represents a floating point literal with a zero fractional part,
+/// return the textual context of the integral part, otherwise `None`.
+llvm::Optional<std::string>
+stripFloatLiteralFraction(const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &Node);
+
+/// Possibly further simplify a duration factory function's argument, without
+/// changing the scale of the factory function. Return that simplification or
+/// the text of the argument if no simplification is possible.
+std::string
+simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result,
+ const Expr &Node);
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+ DurationConversionFunction) {
+ using namespace clang::ast_matchers;
+ return functionDecl(
+ hasAnyName("::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+ "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
+ "::absl::ToDoubleMicroseconds", "::absl::ToDoubleNanoseconds",
+ "::absl::ToInt64Hours", "::absl::ToInt64Minutes",
+ "::absl::ToInt64Seconds", "::absl::ToInt64Milliseconds",
+ "::absl::ToInt64Microseconds", "::absl::ToInt64Nanoseconds"));
+}
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+ DurationFactoryFunction) {
+ using namespace clang::ast_matchers;
+ return functionDecl(hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
+ "::absl::Milliseconds", "::absl::Seconds",
+ "::absl::Minutes", "::absl::Hours"));
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H
Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=348161&r1=348160&r2=348161&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Dec 3 10:35:56 2018
@@ -67,6 +67,12 @@ The improvements are...
Improvements to clang-tidy
--------------------------
+- New :doc:`abseil-duration-comparison
+ <clang-tidy/checks/abseil-duration-comparison>` check.
+
+ Checks for comparisons which should be done in the ``absl::Duration`` domain
+ instead of the float of integer domains.
+
- New :doc:`abseil-duration-division
<clang-tidy/checks/abseil-duration-division>` check.
Added: clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst?rev=348161&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst Mon Dec 3 10:35:56 2018
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - abseil-duration-comparison
+
+abseil-duration-comparison
+==========================
+
+Checks for comparisons which should be in the ``absl::Duration`` domain instead
+of the floating point or integer domains.
+
+N.B.: In cases where a ``Duration`` was being converted to an integer and then
+compared against a floating-point value, truncation during the ``Duration``
+conversion might yield a different result. In practice this is very rare, and
+still indicates a bug which should be fixed.
+
+Examples:
+
+.. code-block:: c++
+
+ // Original - Comparison in the floating point domain
+ double x;
+ absl::Duration d;
+ if (x < absl::ToDoubleSeconds(d)) ...
+
+ // Suggested - Compare in the absl::Duration domain instead
+ if (absl::Seconds(x) < d) ...
+
+
+ // Original - Comparison in the integer domain
+ int x;
+ absl::Duration d;
+ if (x < absl::ToInt64Microseconds(d)) ...
+
+ // Suggested - Compare in the absl::Duration domain instead
+ if (absl::Microseconds(x) < d) ...
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=348161&r1=348160&r2=348161&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Mon Dec 3 10:35:56 2018
@@ -4,6 +4,7 @@ Clang-Tidy Checks
=================
.. toctree::
+ abseil-duration-comparison
abseil-duration-division
abseil-duration-factory-float
abseil-duration-factory-scale
Added: clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp?rev=348161&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp Mon Dec 3 10:35:56 2018
@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+class Time{};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n); \
+ template <typename T> \
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+using int64_t = long long int;
+
+double ToDoubleHours(Duration d);
+double ToDoubleMinutes(Duration d);
+double ToDoubleSeconds(Duration d);
+double ToDoubleMilliseconds(Duration d);
+double ToDoubleMicroseconds(Duration d);
+double ToDoubleNanoseconds(Duration d);
+int64_t ToInt64Hours(Duration d);
+int64_t ToInt64Minutes(Duration d);
+int64_t ToInt64Seconds(Duration d);
+int64_t ToInt64Milliseconds(Duration d);
+int64_t ToInt64Microseconds(Duration d);
+int64_t ToInt64Nanoseconds(Duration d);
+
+// Relational Operators
+constexpr bool operator<(Duration lhs, Duration rhs);
+constexpr bool operator>(Duration lhs, Duration rhs);
+constexpr bool operator>=(Duration lhs, Duration rhs);
+constexpr bool operator<=(Duration lhs, Duration rhs);
+constexpr bool operator==(Duration lhs, Duration rhs);
+constexpr bool operator!=(Duration lhs, Duration rhs);
+
+// Additive Operators
+inline Time operator+(Time lhs, Duration rhs);
+inline Time operator+(Duration lhs, Time rhs);
+inline Time operator-(Time lhs, Duration rhs);
+inline Duration operator-(Time lhs, Time rhs);
+
+} // namespace absl
+
+void f() {
+ double x;
+ absl::Duration d1, d2;
+ bool b;
+ absl::Time t1, t2;
+
+ // Check against the RHS
+ b = x > absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) > d1;
+ b = x >= absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) >= d1;
+ b = x == absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) == d1;
+ b = x <= absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) <= d1;
+ b = x < absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) < d1;
+ b = x == absl::ToDoubleSeconds(t1 - t2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+ b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 > d2;
+
+ // Check against the LHS
+ b = absl::ToDoubleSeconds(d1) < x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 < absl::Seconds(x);
+ b = absl::ToDoubleSeconds(d1) <= x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 <= absl::Seconds(x);
+ b = absl::ToDoubleSeconds(d1) == x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 == absl::Seconds(x);
+ b = absl::ToDoubleSeconds(d1) >= x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 >= absl::Seconds(x);
+ b = absl::ToDoubleSeconds(d1) > x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 > absl::Seconds(x);
+
+ // Comparison against zero
+ b = absl::ToDoubleSeconds(d1) < 0.0;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 < absl::ZeroDuration();
+ b = absl::ToDoubleSeconds(d1) < 0;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: d1 < absl::ZeroDuration();
+
+ // Scales other than Seconds
+ b = x > absl::ToDoubleMicroseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Microseconds(x) > d1;
+ b = x >= absl::ToDoubleMilliseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Milliseconds(x) >= d1;
+ b = x == absl::ToDoubleNanoseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Nanoseconds(x) == d1;
+ b = x <= absl::ToDoubleMinutes(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Minutes(x) <= d1;
+ b = x < absl::ToDoubleHours(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Hours(x) < d1;
+
+ // Integer comparisons
+ b = x > absl::ToInt64Microseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Microseconds(x) > d1;
+ b = x >= absl::ToInt64Milliseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Milliseconds(x) >= d1;
+ b = x == absl::ToInt64Nanoseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Nanoseconds(x) == d1;
+ b = x == absl::ToInt64Seconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(x) == d1;
+ b = x <= absl::ToInt64Minutes(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Minutes(x) <= d1;
+ b = x < absl::ToInt64Hours(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Hours(x) < d1;
+
+ // Other abseil-duration checks folded into this one
+ b = static_cast<double>(5) > absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(5) > d1;
+ b = double(5) > absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(5) > d1;
+ b = float(5) > absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(5) > d1;
+ b = ((double)5) > absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(5) > d1;
+ b = 5.0 > absl::ToDoubleSeconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Seconds(5) > d1;
+
+ // A long expression
+ bool some_condition;
+ int very_very_very_very_long_variable_name;
+ absl::Duration SomeDuration;
+ if (some_condition && very_very_very_very_long_variable_name
+ < absl::ToDoubleSeconds(SomeDuration)) {
+ // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: if (some_condition && absl::Seconds(very_very_very_very_long_variable_name) < SomeDuration) {
+ return;
+ }
+
+ // A complex expression
+ int y;
+ b = (y + 5) * 10 > absl::ToDoubleMilliseconds(d1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1;
+
+ // These should not match
+ b = 6 < 4;
+
+#define TODOUBLE(x) absl::ToDoubleSeconds(x)
+ b = 5.0 > TODOUBLE(d1);
+#undef TODOUBLE
+#define THIRTY 30.0
+ b = THIRTY > absl::ToDoubleSeconds(d1);
+#undef THIRTY
+}
More information about the cfe-commits
mailing list