Skip to content

Commit 00f1d76

Browse files
JonasTothJonasToth
committedDec 3, 2018
[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 llvm-svn: 348169
1 parent 2accb31 commit 00f1d76

12 files changed

+657
-93
lines changed
 

‎clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp

+3
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "../ClangTidy.h"
1111
#include "../ClangTidyModule.h"
1212
#include "../ClangTidyModuleRegistry.h"
13+
#include "DurationComparisonCheck.h"
1314
#include "DurationDivisionCheck.h"
1415
#include "DurationFactoryFloatCheck.h"
1516
#include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@ namespace abseil {
2728
class AbseilModule : public ClangTidyModule {
2829
public:
2930
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
31+
CheckFactories.registerCheck<DurationComparisonCheck>(
32+
"abseil-duration-comparison");
3033
CheckFactories.registerCheck<DurationDivisionCheck>(
3134
"abseil-duration-division");
3235
CheckFactories.registerCheck<DurationFactoryFloatCheck>(

‎clang-tools-extra/clang-tidy/abseil/CMakeLists.txt

+2
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@ set(LLVM_LINK_COMPONENTS support)
22

33
add_clang_library(clangTidyAbseilModule
44
AbseilTidyModule.cpp
5+
DurationComparisonCheck.cpp
56
DurationDivisionCheck.cpp
67
DurationFactoryFloatCheck.cpp
78
DurationFactoryScaleCheck.cpp
9+
DurationRewriter.cpp
810
FasterStrsplitDelimiterCheck.cpp
911
NoInternalDependenciesCheck.cpp
1012
NoNamespaceCheck.cpp

‎clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp

+165
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
//===--- DurationComparisonCheck.cpp - clang-tidy -------------------------===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#include "DurationComparisonCheck.h"
11+
#include "DurationRewriter.h"
12+
#include "clang/AST/ASTContext.h"
13+
#include "clang/ASTMatchers/ASTMatchFinder.h"
14+
#include "clang/Tooling/FixIt.h"
15+
16+
using namespace clang::ast_matchers;
17+
18+
namespace clang {
19+
namespace tidy {
20+
namespace abseil {
21+
22+
/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
23+
/// return its `DurationScale`, or `None` if a match is not found.
24+
static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
25+
static const llvm::DenseMap<llvm::StringRef, DurationScale> ScaleMap(
26+
{{"ToDoubleHours", DurationScale::Hours},
27+
{"ToInt64Hours", DurationScale::Hours},
28+
{"ToDoubleMinutes", DurationScale::Minutes},
29+
{"ToInt64Minutes", DurationScale::Minutes},
30+
{"ToDoubleSeconds", DurationScale::Seconds},
31+
{"ToInt64Seconds", DurationScale::Seconds},
32+
{"ToDoubleMilliseconds", DurationScale::Milliseconds},
33+
{"ToInt64Milliseconds", DurationScale::Milliseconds},
34+
{"ToDoubleMicroseconds", DurationScale::Microseconds},
35+
{"ToInt64Microseconds", DurationScale::Microseconds},
36+
{"ToDoubleNanoseconds", DurationScale::Nanoseconds},
37+
{"ToInt64Nanoseconds", DurationScale::Nanoseconds}});
38+
39+
auto ScaleIter = ScaleMap.find(std::string(Name));
40+
if (ScaleIter == ScaleMap.end())
41+
return llvm::None;
42+
43+
return ScaleIter->second;
44+
}
45+
46+
/// Given a `Scale` return the inverse functions for it.
47+
static const std::pair<llvm::StringRef, llvm::StringRef> &
48+
getInverseForScale(DurationScale Scale) {
49+
static const std::unordered_map<DurationScale,
50+
std::pair<llvm::StringRef, llvm::StringRef>,
51+
std::hash<std::int8>>
52+
InverseMap(
53+
{{DurationScale::Hours,
54+
std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")},
55+
{DurationScale::Minutes, std::make_pair("::absl::ToDoubleMinutes",
56+
"::absl::ToInt64Minutes")},
57+
{DurationScale::Seconds, std::make_pair("::absl::ToDoubleSeconds",
58+
"::absl::ToInt64Seconds")},
59+
{DurationScale::Milliseconds,
60+
std::make_pair("::absl::ToDoubleMilliseconds",
61+
"::absl::ToInt64Milliseconds")},
62+
{DurationScale::Microseconds,
63+
std::make_pair("::absl::ToDoubleMicroseconds",
64+
"::absl::ToInt64Microseconds")},
65+
{DurationScale::Nanoseconds,
66+
std::make_pair("::absl::ToDoubleNanoseconds",
67+
"::absl::ToInt64Nanoseconds")}});
68+
69+
// We know our map contains all the Scale values, so we can skip the
70+
// nonexistence check.
71+
auto InverseIter = InverseMap.find(Scale);
72+
assert(InverseIter != InverseMap.end() && "Unexpected scale found");
73+
return InverseIter->second;
74+
}
75+
76+
/// If `Node` is a call to the inverse of `Scale`, return that inverse's
77+
/// argument, otherwise None.
78+
static llvm::Optional<std::string>
79+
maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
80+
DurationScale Scale, const Expr &Node) {
81+
const std::pair<std::string, std::string> &InverseFunctions =
82+
getInverseForScale(Scale);
83+
if (const Expr *MaybeCallArg = selectFirst<const Expr>(
84+
"e", match(callExpr(callee(functionDecl(
85+
hasAnyName(InverseFunctions.first.c_str(),
86+
InverseFunctions.second.c_str()))),
87+
hasArgument(0, expr().bind("e"))),
88+
Node, *Result.Context))) {
89+
return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
90+
}
91+
92+
return llvm::None;
93+
}
94+
95+
/// Assuming `Node` has type `double` or `int` representing a time interval of
96+
/// `Scale`, return the expression to make it a suitable `Duration`.
97+
static llvm::Optional<std::string> rewriteExprFromNumberToDuration(
98+
const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
99+
const Expr *Node) {
100+
const Expr &RootNode = *Node->IgnoreParenImpCasts();
101+
102+
if (RootNode.getBeginLoc().isMacroID())
103+
return llvm::None;
104+
105+
// First check to see if we can undo a complimentary function call.
106+
if (llvm::Optional<std::string> MaybeRewrite =
107+
maybeRewriteInverseDurationCall(Result, Scale, RootNode))
108+
return *MaybeRewrite;
109+
110+
if (IsLiteralZero(Result, RootNode))
111+
return std::string("absl::ZeroDuration()");
112+
113+
return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
114+
simplifyDurationFactoryArg(Result, RootNode) + ")")
115+
.str();
116+
}
117+
118+
void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
119+
auto Matcher =
120+
binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
121+
hasOperatorName("=="), hasOperatorName("<="),
122+
hasOperatorName("<")),
123+
hasEitherOperand(ignoringImpCasts(callExpr(
124+
callee(functionDecl(DurationConversionFunction())
125+
.bind("function_decl"))))))
126+
.bind("binop");
127+
128+
Finder->addMatcher(Matcher, this);
129+
}
130+
131+
void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
132+
const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
133+
134+
// Don't try to replace things inside of macro definitions.
135+
if (Binop->getExprLoc().isMacroID())
136+
return;
137+
138+
llvm::Optional<DurationScale> Scale = getScaleForInverse(
139+
Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
140+
if (!Scale)
141+
return;
142+
143+
// In most cases, we'll only need to rewrite one of the sides, but we also
144+
// want to handle the case of rewriting both sides. This is much simpler if
145+
// we unconditionally try and rewrite both, and let the rewriter determine
146+
// if nothing needs to be done.
147+
llvm::Optional<std::string> LhsReplacement =
148+
rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
149+
llvm::Optional<std::string> RhsReplacement =
150+
rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
151+
152+
if (!(LhsReplacement && RhsReplacement))
153+
return;
154+
155+
diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
156+
<< FixItHint::CreateReplacement(Binop->getSourceRange(),
157+
(llvm::Twine(*LhsReplacement) + " " +
158+
Binop->getOpcodeStr() + " " +
159+
*RhsReplacement)
160+
.str());
161+
}
162+
163+
} // namespace abseil
164+
} // namespace tidy
165+
} // namespace clang

‎clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.h

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
//===--- DurationComparisonCheck.h - clang-tidy -----------------*- C++ -*-===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H
11+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H
12+
13+
#include "../ClangTidy.h"
14+
15+
namespace clang {
16+
namespace tidy {
17+
namespace abseil {
18+
19+
/// Prefer comparison in the absl::Duration domain instead of the numeric
20+
/// domain.
21+
///
22+
/// For the user-facing documentation see:
23+
/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-comparison.html
24+
class DurationComparisonCheck : public ClangTidyCheck {
25+
public:
26+
DurationComparisonCheck(StringRef Name, ClangTidyContext *Context)
27+
: ClangTidyCheck(Name, Context) {}
28+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
29+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
30+
};
31+
32+
} // namespace abseil
33+
} // namespace tidy
34+
} // namespace clang
35+
36+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H

‎clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.cpp

+15-49
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
//===----------------------------------------------------------------------===//
99

1010
#include "DurationFactoryFloatCheck.h"
11+
#include "DurationRewriter.h"
1112
#include "clang/AST/ASTContext.h"
1213
#include "clang/ASTMatchers/ASTMatchFinder.h"
1314
#include "clang/Tooling/FixIt.h"
@@ -18,19 +19,6 @@ namespace clang {
1819
namespace tidy {
1920
namespace abseil {
2021

21-
// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
22-
static llvm::Optional<llvm::APSInt>
23-
truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
24-
double Value = FloatLiteral.getValueAsApproximateDouble();
25-
if (std::fmod(Value, 1) == 0) {
26-
if (Value >= static_cast<double>(1u << 31))
27-
return llvm::None;
28-
29-
return llvm::APSInt::get(static_cast<int64_t>(Value));
30-
}
31-
return llvm::None;
32-
}
33-
3422
// Returns `true` if `Range` is inside a macro definition.
3523
static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
3624
SourceRange Range) {
@@ -42,21 +30,14 @@ static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
4230

4331
void DurationFactoryFloatCheck::registerMatchers(MatchFinder *Finder) {
4432
Finder->addMatcher(
45-
callExpr(
46-
callee(functionDecl(hasAnyName(
47-
"absl::Nanoseconds", "absl::Microseconds", "absl::Milliseconds",
48-
"absl::Seconds", "absl::Minutes", "absl::Hours"))),
49-
hasArgument(0,
50-
anyOf(cxxStaticCastExpr(
51-
hasDestinationType(realFloatingPointType()),
52-
hasSourceExpression(expr().bind("cast_arg"))),
53-
cStyleCastExpr(
54-
hasDestinationType(realFloatingPointType()),
55-
hasSourceExpression(expr().bind("cast_arg"))),
56-
cxxFunctionalCastExpr(
57-
hasDestinationType(realFloatingPointType()),
58-
hasSourceExpression(expr().bind("cast_arg"))),
59-
floatLiteral().bind("float_literal"))))
33+
callExpr(callee(functionDecl(DurationFactoryFunction())),
34+
hasArgument(0, anyOf(cxxStaticCastExpr(hasDestinationType(
35+
realFloatingPointType())),
36+
cStyleCastExpr(hasDestinationType(
37+
realFloatingPointType())),
38+
cxxFunctionalCastExpr(hasDestinationType(
39+
realFloatingPointType())),
40+
floatLiteral())))
6041
.bind("call"),
6142
this);
6243
}
@@ -73,31 +54,16 @@ void DurationFactoryFloatCheck::check(const MatchFinder::MatchResult &Result) {
7354
if (Arg->getBeginLoc().isMacroID())
7455
return;
7556

76-
// Check for casts to `float` or `double`.
77-
if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) {
57+
llvm::Optional<std::string> SimpleArg = stripFloatCast(Result, *Arg);
58+
if (!SimpleArg)
59+
SimpleArg = stripFloatLiteralFraction(Result, *Arg);
60+
61+
if (SimpleArg) {
7862
diag(MatchedCall->getBeginLoc(),
7963
(llvm::Twine("use the integer version of absl::") +
8064
MatchedCall->getDirectCallee()->getName())
8165
.str())
82-
<< FixItHint::CreateReplacement(
83-
Arg->getSourceRange(),
84-
tooling::fixit::getText(*MaybeCastArg, *Result.Context));
85-
return;
86-
}
87-
88-
// Check for floats without fractional components.
89-
if (const auto *LitFloat =
90-
Result.Nodes.getNodeAs<FloatingLiteral>("float_literal")) {
91-
// Attempt to simplify a `Duration` factory call with a literal argument.
92-
if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat)) {
93-
diag(MatchedCall->getBeginLoc(),
94-
(llvm::Twine("use the integer version of absl::") +
95-
MatchedCall->getDirectCallee()->getName())
96-
.str())
97-
<< FixItHint::CreateReplacement(LitFloat->getSourceRange(),
98-
IntValue->toString(/*radix=*/10));
99-
return;
100-
}
66+
<< FixItHint::CreateReplacement(Arg->getSourceRange(), *SimpleArg);
10167
}
10268
}
10369

‎clang-tools-extra/clang-tidy/abseil/DurationFactoryScaleCheck.cpp

+6-44
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
//===----------------------------------------------------------------------===//
99

1010
#include "DurationFactoryScaleCheck.h"
11+
#include "DurationRewriter.h"
1112
#include "clang/AST/ASTContext.h"
1213
#include "clang/ASTMatchers/ASTMatchFinder.h"
1314
#include "clang/Tooling/FixIt.h"
@@ -18,20 +19,6 @@ namespace clang {
1819
namespace tidy {
1920
namespace abseil {
2021

21-
namespace {
22-
23-
// Potential scales of our inputs.
24-
enum class DurationScale {
25-
Hours,
26-
Minutes,
27-
Seconds,
28-
Milliseconds,
29-
Microseconds,
30-
Nanoseconds,
31-
};
32-
33-
} // namespace
34-
3522
// Given the name of a duration factory function, return the appropriate
3623
// `DurationScale` for that factory. If no factory can be found for
3724
// `FactoryName`, return `None`.
@@ -129,39 +116,14 @@ static llvm::Optional<DurationScale> GetNewScale(DurationScale OldScale,
129116
return llvm::None;
130117
}
131118

132-
// Given a `Scale`, return the appropriate factory function call for
133-
// constructing a `Duration` for that scale.
134-
static llvm::StringRef GetFactoryForScale(DurationScale Scale) {
135-
switch (Scale) {
136-
case DurationScale::Hours:
137-
return "absl::Hours";
138-
case DurationScale::Minutes:
139-
return "absl::Minutes";
140-
case DurationScale::Seconds:
141-
return "absl::Seconds";
142-
case DurationScale::Milliseconds:
143-
return "absl::Milliseconds";
144-
case DurationScale::Microseconds:
145-
return "absl::Microseconds";
146-
case DurationScale::Nanoseconds:
147-
return "absl::Nanoseconds";
148-
}
149-
llvm_unreachable("unknown scaling factor");
150-
}
151-
152119
void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) {
153120
Finder->addMatcher(
154121
callExpr(
155-
callee(functionDecl(
156-
hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
157-
"::absl::Milliseconds", "::absl::Seconds",
158-
"::absl::Minutes", "::absl::Hours"))
159-
.bind("call_decl")),
122+
callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
160123
hasArgument(
161124
0,
162125
ignoringImpCasts(anyOf(
163-
integerLiteral(equals(0)).bind("zero"),
164-
floatLiteral(equals(0.0)).bind("zero"),
126+
integerLiteral(equals(0)), floatLiteral(equals(0.0)),
165127
binaryOperator(hasOperatorName("*"),
166128
hasEitherOperand(ignoringImpCasts(
167129
anyOf(integerLiteral(), floatLiteral()))))
@@ -185,7 +147,7 @@ void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
185147
return;
186148

187149
// We first handle the cases of literal zero (both float and integer).
188-
if (Result.Nodes.getNodeAs<Stmt>("zero")) {
150+
if (IsLiteralZero(Result, *Arg)) {
189151
diag(Call->getBeginLoc(),
190152
"use ZeroDuration() for zero-length time intervals")
191153
<< FixItHint::CreateReplacement(Call->getSourceRange(),
@@ -244,7 +206,7 @@ void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
244206
diag(Call->getBeginLoc(), "internal duration scaling can be removed")
245207
<< FixItHint::CreateReplacement(
246208
Call->getSourceRange(),
247-
(llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
209+
(llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
248210
tooling::fixit::getText(*Remainder, *Result.Context) + ")")
249211
.str());
250212
}
@@ -257,7 +219,7 @@ void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
257219
diag(Call->getBeginLoc(), "internal duration scaling can be removed")
258220
<< FixItHint::CreateReplacement(
259221
Call->getSourceRange(),
260-
(llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
222+
(llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
261223
tooling::fixit::getText(*Remainder, *Result.Context) + ")")
262224
.str());
263225
}

‎clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp

+109
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
//===--- DurationRewriter.cpp - clang-tidy --------------------------------===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#include "DurationRewriter.h"
11+
#include "clang/Tooling/FixIt.h"
12+
13+
using namespace clang::ast_matchers;
14+
15+
namespace clang {
16+
namespace tidy {
17+
namespace abseil {
18+
19+
/// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
20+
static llvm::Optional<llvm::APSInt>
21+
truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
22+
double Value = FloatLiteral.getValueAsApproximateDouble();
23+
if (std::fmod(Value, 1) == 0) {
24+
if (Value >= static_cast<double>(1u << 31))
25+
return llvm::None;
26+
27+
return llvm::APSInt::get(static_cast<int64_t>(Value));
28+
}
29+
return llvm::None;
30+
}
31+
32+
/// Returns the factory function name for a given `Scale`.
33+
llvm::StringRef getFactoryForScale(DurationScale Scale) {
34+
switch (Scale) {
35+
case DurationScale::Hours:
36+
return "absl::Hours";
37+
case DurationScale::Minutes:
38+
return "absl::Minutes";
39+
case DurationScale::Seconds:
40+
return "absl::Seconds";
41+
case DurationScale::Milliseconds:
42+
return "absl::Milliseconds";
43+
case DurationScale::Microseconds:
44+
return "absl::Microseconds";
45+
case DurationScale::Nanoseconds:
46+
return "absl::Nanoseconds";
47+
}
48+
llvm_unreachable("unknown scaling factor");
49+
}
50+
51+
/// Returns `true` if `Node` is a value which evaluates to a literal `0`.
52+
bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) {
53+
return selectFirst<const clang::Expr>(
54+
"val",
55+
match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)),
56+
floatLiteral(equals(0.0)))))
57+
.bind("val"),
58+
Node, *Result.Context)) != nullptr;
59+
}
60+
61+
llvm::Optional<std::string>
62+
stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
63+
const Expr &Node) {
64+
if (const Expr *MaybeCastArg = selectFirst<const Expr>(
65+
"cast_arg",
66+
match(expr(anyOf(cxxStaticCastExpr(
67+
hasDestinationType(realFloatingPointType()),
68+
hasSourceExpression(expr().bind("cast_arg"))),
69+
cStyleCastExpr(
70+
hasDestinationType(realFloatingPointType()),
71+
hasSourceExpression(expr().bind("cast_arg"))),
72+
cxxFunctionalCastExpr(
73+
hasDestinationType(realFloatingPointType()),
74+
hasSourceExpression(expr().bind("cast_arg"))))),
75+
Node, *Result.Context)))
76+
return tooling::fixit::getText(*MaybeCastArg, *Result.Context).str();
77+
78+
return llvm::None;
79+
}
80+
81+
llvm::Optional<std::string>
82+
stripFloatLiteralFraction(const MatchFinder::MatchResult &Result,
83+
const Expr &Node) {
84+
if (const auto *LitFloat = llvm::dyn_cast<FloatingLiteral>(&Node))
85+
// Attempt to simplify a `Duration` factory call with a literal argument.
86+
if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat))
87+
return IntValue->toString(/*radix=*/10);
88+
89+
return llvm::None;
90+
}
91+
92+
std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result,
93+
const Expr &Node) {
94+
// Check for an explicit cast to `float` or `double`.
95+
if (llvm::Optional<std::string> MaybeArg = stripFloatCast(Result, Node))
96+
return *MaybeArg;
97+
98+
// Check for floats without fractional components.
99+
if (llvm::Optional<std::string> MaybeArg =
100+
stripFloatLiteralFraction(Result, Node))
101+
return *MaybeArg;
102+
103+
// We couldn't simplify any further, so return the argument text.
104+
return tooling::fixit::getText(Node, *Result.Context).str();
105+
}
106+
107+
} // namespace abseil
108+
} // namespace tidy
109+
} // namespace clang

‎clang-tools-extra/clang-tidy/abseil/DurationRewriter.h

+86
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
//===--- DurationRewriter.h - clang-tidy ------------------------*- C++ -*-===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H
11+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H
12+
13+
#include "clang/ASTMatchers/ASTMatchFinder.h"
14+
#include "clang/ASTMatchers/ASTMatchers.h"
15+
#include <cinttypes>
16+
17+
namespace clang {
18+
namespace tidy {
19+
namespace abseil {
20+
21+
/// Duration factory and conversion scales
22+
enum class DurationScale : std::int8_t {
23+
Hours,
24+
Minutes,
25+
Seconds,
26+
Milliseconds,
27+
Microseconds,
28+
Nanoseconds,
29+
};
30+
31+
/// Given a `Scale`, return the appropriate factory function call for
32+
/// constructing a `Duration` for that scale.
33+
llvm::StringRef getFactoryForScale(DurationScale Scale);
34+
35+
// Determine if `Node` represents a literal floating point or integral zero.
36+
bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result,
37+
const Expr &Node);
38+
39+
/// Possibly strip a floating point cast expression.
40+
///
41+
/// If `Node` represents an explicit cast to a floating point type, return
42+
/// the textual context of the cast argument, otherwise `None`.
43+
llvm::Optional<std::string>
44+
stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
45+
const Expr &Node);
46+
47+
/// Possibly remove the fractional part of a floating point literal.
48+
///
49+
/// If `Node` represents a floating point literal with a zero fractional part,
50+
/// return the textual context of the integral part, otherwise `None`.
51+
llvm::Optional<std::string>
52+
stripFloatLiteralFraction(const ast_matchers::MatchFinder::MatchResult &Result,
53+
const Expr &Node);
54+
55+
/// Possibly further simplify a duration factory function's argument, without
56+
/// changing the scale of the factory function. Return that simplification or
57+
/// the text of the argument if no simplification is possible.
58+
std::string
59+
simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result,
60+
const Expr &Node);
61+
62+
AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
63+
DurationConversionFunction) {
64+
using namespace clang::ast_matchers;
65+
return functionDecl(
66+
hasAnyName("::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
67+
"::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
68+
"::absl::ToDoubleMicroseconds", "::absl::ToDoubleNanoseconds",
69+
"::absl::ToInt64Hours", "::absl::ToInt64Minutes",
70+
"::absl::ToInt64Seconds", "::absl::ToInt64Milliseconds",
71+
"::absl::ToInt64Microseconds", "::absl::ToInt64Nanoseconds"));
72+
}
73+
74+
AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
75+
DurationFactoryFunction) {
76+
using namespace clang::ast_matchers;
77+
return functionDecl(hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
78+
"::absl::Milliseconds", "::absl::Seconds",
79+
"::absl::Minutes", "::absl::Hours"));
80+
}
81+
82+
} // namespace abseil
83+
} // namespace tidy
84+
} // namespace clang
85+
86+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H

‎clang-tools-extra/docs/ReleaseNotes.rst

+6
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,12 @@ The improvements are...
6767
Improvements to clang-tidy
6868
--------------------------
6969

70+
- New :doc:`abseil-duration-comparison
71+
<clang-tidy/checks/abseil-duration-comparison>` check.
72+
73+
Checks for comparisons which should be done in the ``absl::Duration`` domain
74+
instead of the float of integer domains.
75+
7076
- New :doc:`abseil-duration-division
7177
<clang-tidy/checks/abseil-duration-division>` check.
7278

‎clang-tools-extra/docs/clang-tidy/checks/abseil-duration-comparison.rst

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
.. title:: clang-tidy - abseil-duration-comparison
2+
3+
abseil-duration-comparison
4+
==========================
5+
6+
Checks for comparisons which should be in the ``absl::Duration`` domain instead
7+
of the floating point or integer domains.
8+
9+
N.B.: In cases where a ``Duration`` was being converted to an integer and then
10+
compared against a floating-point value, truncation during the ``Duration``
11+
conversion might yield a different result. In practice this is very rare, and
12+
still indicates a bug which should be fixed.
13+
14+
Examples:
15+
16+
.. code-block:: c++
17+
18+
// Original - Comparison in the floating point domain
19+
double x;
20+
absl::Duration d;
21+
if (x < absl::ToDoubleSeconds(d)) ...
22+
23+
// Suggested - Compare in the absl::Duration domain instead
24+
if (absl::Seconds(x) < d) ...
25+
26+
27+
// Original - Comparison in the integer domain
28+
int x;
29+
absl::Duration d;
30+
if (x < absl::ToInt64Microseconds(d)) ...
31+
32+
// Suggested - Compare in the absl::Duration domain instead
33+
if (absl::Microseconds(x) < d) ...

‎clang-tools-extra/docs/clang-tidy/checks/list.rst

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ Clang-Tidy Checks
44
=================
55

66
.. toctree::
7+
abseil-duration-comparison
78
abseil-duration-division
89
abseil-duration-factory-float
910
abseil-duration-factory-scale

‎clang-tools-extra/test/clang-tidy/abseil-duration-comparison.cpp

+195
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
// RUN: %check_clang_tidy %s abseil-duration-comparison %t
2+
3+
// Mimic the implementation of absl::Duration
4+
namespace absl {
5+
6+
class Duration {};
7+
class Time{};
8+
9+
Duration Nanoseconds(long long);
10+
Duration Microseconds(long long);
11+
Duration Milliseconds(long long);
12+
Duration Seconds(long long);
13+
Duration Minutes(long long);
14+
Duration Hours(long long);
15+
16+
#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
17+
Duration NAME(float n); \
18+
Duration NAME(double n); \
19+
template <typename T> \
20+
Duration NAME(T n);
21+
22+
GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
23+
GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
24+
GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
25+
GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
26+
GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
27+
GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
28+
#undef GENERATE_DURATION_FACTORY_OVERLOADS
29+
30+
using int64_t = long long int;
31+
32+
double ToDoubleHours(Duration d);
33+
double ToDoubleMinutes(Duration d);
34+
double ToDoubleSeconds(Duration d);
35+
double ToDoubleMilliseconds(Duration d);
36+
double ToDoubleMicroseconds(Duration d);
37+
double ToDoubleNanoseconds(Duration d);
38+
int64_t ToInt64Hours(Duration d);
39+
int64_t ToInt64Minutes(Duration d);
40+
int64_t ToInt64Seconds(Duration d);
41+
int64_t ToInt64Milliseconds(Duration d);
42+
int64_t ToInt64Microseconds(Duration d);
43+
int64_t ToInt64Nanoseconds(Duration d);
44+
45+
// Relational Operators
46+
constexpr bool operator<(Duration lhs, Duration rhs);
47+
constexpr bool operator>(Duration lhs, Duration rhs);
48+
constexpr bool operator>=(Duration lhs, Duration rhs);
49+
constexpr bool operator<=(Duration lhs, Duration rhs);
50+
constexpr bool operator==(Duration lhs, Duration rhs);
51+
constexpr bool operator!=(Duration lhs, Duration rhs);
52+
53+
// Additive Operators
54+
inline Time operator+(Time lhs, Duration rhs);
55+
inline Time operator+(Duration lhs, Time rhs);
56+
inline Time operator-(Time lhs, Duration rhs);
57+
inline Duration operator-(Time lhs, Time rhs);
58+
59+
} // namespace absl
60+
61+
void f() {
62+
double x;
63+
absl::Duration d1, d2;
64+
bool b;
65+
absl::Time t1, t2;
66+
67+
// Check against the RHS
68+
b = x > absl::ToDoubleSeconds(d1);
69+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
70+
// CHECK-FIXES: absl::Seconds(x) > d1;
71+
b = x >= absl::ToDoubleSeconds(d1);
72+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
73+
// CHECK-FIXES: absl::Seconds(x) >= d1;
74+
b = x == absl::ToDoubleSeconds(d1);
75+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
76+
// CHECK-FIXES: absl::Seconds(x) == d1;
77+
b = x <= absl::ToDoubleSeconds(d1);
78+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
79+
// CHECK-FIXES: absl::Seconds(x) <= d1;
80+
b = x < absl::ToDoubleSeconds(d1);
81+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
82+
// CHECK-FIXES: absl::Seconds(x) < d1;
83+
b = x == absl::ToDoubleSeconds(t1 - t2);
84+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
85+
// CHECK-FIXES: absl::Seconds(x) == t1 - t2;
86+
b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
87+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
88+
// CHECK-FIXES: d1 > d2;
89+
90+
// Check against the LHS
91+
b = absl::ToDoubleSeconds(d1) < x;
92+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
93+
// CHECK-FIXES: d1 < absl::Seconds(x);
94+
b = absl::ToDoubleSeconds(d1) <= x;
95+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
96+
// CHECK-FIXES: d1 <= absl::Seconds(x);
97+
b = absl::ToDoubleSeconds(d1) == x;
98+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
99+
// CHECK-FIXES: d1 == absl::Seconds(x);
100+
b = absl::ToDoubleSeconds(d1) >= x;
101+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
102+
// CHECK-FIXES: d1 >= absl::Seconds(x);
103+
b = absl::ToDoubleSeconds(d1) > x;
104+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
105+
// CHECK-FIXES: d1 > absl::Seconds(x);
106+
107+
// Comparison against zero
108+
b = absl::ToDoubleSeconds(d1) < 0.0;
109+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
110+
// CHECK-FIXES: d1 < absl::ZeroDuration();
111+
b = absl::ToDoubleSeconds(d1) < 0;
112+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
113+
// CHECK-FIXES: d1 < absl::ZeroDuration();
114+
115+
// Scales other than Seconds
116+
b = x > absl::ToDoubleMicroseconds(d1);
117+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
118+
// CHECK-FIXES: absl::Microseconds(x) > d1;
119+
b = x >= absl::ToDoubleMilliseconds(d1);
120+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
121+
// CHECK-FIXES: absl::Milliseconds(x) >= d1;
122+
b = x == absl::ToDoubleNanoseconds(d1);
123+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
124+
// CHECK-FIXES: absl::Nanoseconds(x) == d1;
125+
b = x <= absl::ToDoubleMinutes(d1);
126+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
127+
// CHECK-FIXES: absl::Minutes(x) <= d1;
128+
b = x < absl::ToDoubleHours(d1);
129+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
130+
// CHECK-FIXES: absl::Hours(x) < d1;
131+
132+
// Integer comparisons
133+
b = x > absl::ToInt64Microseconds(d1);
134+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
135+
// CHECK-FIXES: absl::Microseconds(x) > d1;
136+
b = x >= absl::ToInt64Milliseconds(d1);
137+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
138+
// CHECK-FIXES: absl::Milliseconds(x) >= d1;
139+
b = x == absl::ToInt64Nanoseconds(d1);
140+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
141+
// CHECK-FIXES: absl::Nanoseconds(x) == d1;
142+
b = x == absl::ToInt64Seconds(d1);
143+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
144+
// CHECK-FIXES: absl::Seconds(x) == d1;
145+
b = x <= absl::ToInt64Minutes(d1);
146+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
147+
// CHECK-FIXES: absl::Minutes(x) <= d1;
148+
b = x < absl::ToInt64Hours(d1);
149+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
150+
// CHECK-FIXES: absl::Hours(x) < d1;
151+
152+
// Other abseil-duration checks folded into this one
153+
b = static_cast<double>(5) > absl::ToDoubleSeconds(d1);
154+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
155+
// CHECK-FIXES: absl::Seconds(5) > d1;
156+
b = double(5) > absl::ToDoubleSeconds(d1);
157+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
158+
// CHECK-FIXES: absl::Seconds(5) > d1;
159+
b = float(5) > absl::ToDoubleSeconds(d1);
160+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
161+
// CHECK-FIXES: absl::Seconds(5) > d1;
162+
b = ((double)5) > absl::ToDoubleSeconds(d1);
163+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
164+
// CHECK-FIXES: absl::Seconds(5) > d1;
165+
b = 5.0 > absl::ToDoubleSeconds(d1);
166+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
167+
// CHECK-FIXES: absl::Seconds(5) > d1;
168+
169+
// A long expression
170+
bool some_condition;
171+
int very_very_very_very_long_variable_name;
172+
absl::Duration SomeDuration;
173+
if (some_condition && very_very_very_very_long_variable_name
174+
< absl::ToDoubleSeconds(SomeDuration)) {
175+
// CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the duration domain [abseil-duration-comparison]
176+
// CHECK-FIXES: if (some_condition && absl::Seconds(very_very_very_very_long_variable_name) < SomeDuration) {
177+
return;
178+
}
179+
180+
// A complex expression
181+
int y;
182+
b = (y + 5) * 10 > absl::ToDoubleMilliseconds(d1);
183+
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
184+
// CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1;
185+
186+
// These should not match
187+
b = 6 < 4;
188+
189+
#define TODOUBLE(x) absl::ToDoubleSeconds(x)
190+
b = 5.0 > TODOUBLE(d1);
191+
#undef TODOUBLE
192+
#define THIRTY 30.0
193+
b = THIRTY > absl::ToDoubleSeconds(d1);
194+
#undef THIRTY
195+
}

0 commit comments

Comments
 (0)
Please sign in to comment.