[clang-tools-extra] r348169 - [clang-tidy] Recommit: Add the abseil-duration-comparison check
Jonas Toth via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 3 11:22:09 PST 2018
Author: jonastoth
Date: Mon Dec 3 11:22:08 2018
New Revision: 348169
URL: http://llvm.org/viewvc/llvm-project?rev=348169&view=rev
Log:
[clang-tidy] Recommit: 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.
Compilation is unbroken, because the hash-function is now directly
specified for std::unordered_map, as 'enum class' does not compile as
key (seamingly only on some compilers).
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=348169&r1=348168&r2=348169&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp Mon Dec 3 11:22:08 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=348169&r1=348168&r2=348169&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt Mon Dec 3 11:22:08 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=348169&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Mon Dec 3 11:22:08 2018
@@ -0,0 +1,165 @@
+//===--- 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>,
+ std::hash<std::int8>>
+ 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=348169&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h Mon Dec 3 11:22:08 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=348169&r1=348168&r2=348169&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp Mon Dec 3 11:22:08 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=348169&r1=348168&r2=348169&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp Mon Dec 3 11:22:08 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=348169&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp Mon Dec 3 11:22:08 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=348169&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h Mon Dec 3 11:22:08 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=348169&r1=348168&r2=348169&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Dec 3 11:22:08 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=348169&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 11:22:08 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=348169&r1=348168&r2=348169&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 11:22:08 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=348169&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 11:22:08 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