hwright updated this revision to Diff 173543.
hwright marked 16 inline comments as done.
hwright added a comment.
Addressed reviewer feedback.
https://reviews.llvm.org/D54246
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tidy/abseil/DurationFactoryScaleCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-scale.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-scale.cpp
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+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
+
+} // namespace absl
+
+void ScaleTest() {
+ absl::Duration d;
+
+ // Zeroes
+ d = absl::Hours(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Minutes(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Seconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Milliseconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Microseconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Nanoseconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Seconds(0.0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+
+ // Fold seconds into minutes
+ d = absl::Seconds(30 * 60);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(30);
+ d = absl::Seconds(60 * 30);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(30);
+
+ // Try a few more exotic multiplications
+ d = absl::Seconds(60 * 30 * 60);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(60 * 30);
+ d = absl::Seconds(1e-3 * 30);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Milliseconds(30);
+ d = absl::Milliseconds(30 * 1000);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Seconds(30);
+ d = absl::Milliseconds(30 * 0.001);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Microseconds(30);
+
+ // Division
+ d = absl::Hours(30 / 60.);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(30);
+ d = absl::Seconds(30 / 1000.);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Milliseconds(30);
+ d = absl::Milliseconds(30 / 1e3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Microseconds(30);
+
+ // None of these should trigger the check
+ d = absl::Seconds(60);
+ d = absl::Seconds(60 + 30);
+ d = absl::Seconds(60 - 30);
+ d = absl::Seconds(50 * 30);
+ d = absl::Hours(60 * 60);
+ d = absl::Nanoseconds(1e-3 * 30);
+ d = absl::Seconds(1000 / 30);
+ // We don't support division by integers, which could cause rounding
+ d = absl::Seconds(10 / 1000);
+ d = absl::Seconds(30 / 50);
+
+#define EXPRESSION 30 * 60
+ d = absl::Seconds(EXPRESSION);
+#undef EXPRESSION
+
+// This should not trigger
+#define HOURS(x) absl::Minutes(60 * x)
+ d = HOURS(40);
+#undef HOURS
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
abseil-duration-division
abseil-duration-factory-float
+ abseil-duration-factory-scale
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
Index: docs/clang-tidy/checks/abseil-duration-factory-scale.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/abseil-duration-factory-scale.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - abseil-duration-factory-scale
+
+abseil-duration-factory-scale
+=============================
+
+Checks for cases where arguments to ``absl::Duration`` factory functions are
+scaled internally and could be changed to a different factory function. This
+check also looks for arguements with a zero value and suggests using
+``absl::ZeroDuration()`` instead.
+
+Examples:
+
+.. code-block:: c++
+
+ // Original - Internal multiplication.
+ int x;
+ absl::Duration d = absl::Seconds(60 * x);
+
+ // Suggested - Use absl::Minutes instead.
+ absl::Duration d = absl::Minutes(x);
+
+
+ // Original - Internal division.
+ int y;
+ absl::Duration d = absl::Milliseconds(y / 1000.);
+
+ // Suggested - Use absl:::Seconds instead.
+ absl::Duration d = absl::Seconds(y);
+
+
+ // Original - Zero-value argument.
+ absl::Duration d = absl::Hours(0);
+
+ // Suggested = Use absl::ZeroDuration instead
+ absl::Duration d = absl::ZeroDuration();
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -81,6 +81,12 @@
``absl::Duration`` factory functions are called when the more-efficient
integer versions could be used instead.
+- New :doc:`abseil-duration-factory-scale
+ <clang-tidy/checks/abseil-duration-factory-scale>` check.
+
+ Checks for cases where arguments to ``absl::Duration`` factory functions are
+ scaled internally and could be changed to a different factory function.
+
- New :doc:`abseil-faster-strsplit-delimiter
<clang-tidy/checks/abseil-faster-strsplit-delimiter>` check.
Index: clang-tidy/abseil/DurationFactoryScaleCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/abseil/DurationFactoryScaleCheck.h
@@ -0,0 +1,38 @@
+//===--- DurationFactoryScaleCheck.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_DURATIONFACTORYSCALECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYSCALECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check finds cases where the incorrect `Duration` factory funciton is
+/// being used by looking for scaling constants inside the factory argument
+/// and suggesting a more appropriate factory. It also looks for the special
+/// case of zero and suggests `ZeroDuration()`.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-factory-scale.html
+class DurationFactoryScaleCheck : public ClangTidyCheck {
+public:
+ DurationFactoryScaleCheck(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_DURATIONFACTORYSCALECHECK_H
Index: clang-tidy/abseil/DurationFactoryScaleCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/abseil/DurationFactoryScaleCheck.cpp
@@ -0,0 +1,286 @@
+//===--- DurationFactoryScaleCheck.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 "DurationFactoryScaleCheck.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 {
+
+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`.
+static llvm::Optional<DurationScale>
+GetScaleForFactory(llvm::StringRef FactoryName) {
+ static const std::unordered_map<std::string, DurationScale> ScaleMap(
+ {{"Nanoseconds", DurationScale::Nanoseconds},
+ {"Microseconds", DurationScale::Microseconds},
+ {"Milliseconds", DurationScale::Milliseconds},
+ {"Seconds", DurationScale::Seconds},
+ {"Minutes", DurationScale::Minutes},
+ {"Hours", DurationScale::Hours}});
+
+ const auto ScaleIter = ScaleMap.find(FactoryName);
+ if (ScaleIter == ScaleMap.end())
+ return llvm::None;
+
+ return ScaleIter->second;
+}
+
+// Given either an integer or float literal, return it's value.
+// One and only one of `IntLit` and `FloatLit` should be provided.
+static double GetValue(const IntegerLiteral *IntLit,
+ const FloatingLiteral *FloatLit) {
+ if (IntLit) {
+ return IntLit->getValue().getLimitedValue();
+ }
+ assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+ return FloatLit->getValueAsApproximateDouble();
+}
+
+// Given the scale of a duration and a `Multiplier`, determine if `Multiplier`
+// would produce a new scale. If so, return it, otherwise `None`.
+static llvm::Optional<DurationScale> GetNewMultScale(DurationScale OldScale,
+ double Multiplier) {
+ switch (OldScale) {
+ case DurationScale::Hours:
+ // We can't scale Hours.
+ break;
+
+ case DurationScale::Minutes:
+ if (Multiplier == 60.0)
+ return DurationScale::Hours;
+ break;
+
+ case DurationScale::Seconds:
+ if (Multiplier == 60.0)
+ return DurationScale::Minutes;
+ if (Multiplier == 1e-3)
+ return DurationScale::Milliseconds;
+ break;
+
+ case DurationScale::Milliseconds:
+ if (Multiplier == 1e3)
+ return DurationScale::Seconds;
+ if (Multiplier == 1e-3)
+ return DurationScale::Microseconds;
+ break;
+
+ case DurationScale::Microseconds:
+ if (Multiplier == 1e3)
+ return DurationScale::Milliseconds;
+ if (Multiplier == 1e-6)
+ return DurationScale::Nanoseconds;
+ break;
+
+ case DurationScale::Nanoseconds:
+ if (Multiplier == 1e3)
+ return DurationScale::Microseconds;
+ break;
+ }
+
+ return llvm::None;
+}
+
+// Given the scale of a duration and `Divisor`, determine if division by
+// `Divisor` would produce a new scale. If so, return it, otherwise `None`.
+static llvm::Optional<DurationScale> GetNewDivScale(DurationScale OldScale,
+ double Divisor) {
+ switch (OldScale) {
+ case DurationScale::Hours:
+ if (Divisor == 60.0)
+ return DurationScale::Minutes;
+ break;
+
+ case DurationScale::Minutes:
+ if (Divisor == 60.0)
+ return DurationScale::Seconds;
+ break;
+
+ case DurationScale::Seconds:
+ if (Divisor == 1e3)
+ return DurationScale::Milliseconds;
+ break;
+
+ case DurationScale::Milliseconds:
+ if (Divisor == 1e3)
+ return DurationScale::Microseconds;
+ break;
+
+ case DurationScale::Microseconds:
+ if (Divisor == 1e3)
+ return DurationScale::Nanoseconds;
+ break;
+
+ case DurationScale::Nanoseconds:
+ // We can't scale Nanoseconds down.
+ break;
+ }
+
+ return llvm::None;
+}
+
+// Given a `Scale`, return the appropriate factory function call for
+// constructing a `Duration` for that scale.
+static std::string 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();
+}
+
+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")),
+ hasArgument(
+ 0,
+ ignoringImpCasts(anyOf(
+ integerLiteral(equals(0)).bind("zero"),
+ floatLiteral(equals(0.0)).bind("zero"),
+ binaryOperator(hasOperatorName("*"),
+ hasEitherOperand(ignoringImpCasts(
+ anyOf(integerLiteral(), floatLiteral()))))
+ .bind("mult_binop"),
+ binaryOperator(hasOperatorName("/"), hasRHS(floatLiteral()))
+ .bind("div_binop")))))
+ .bind("call"),
+ this);
+}
+
+void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+
+ // Don't try and replace things inside of macro definitions.
+ if (Call->getExprLoc().isMacroID())
+ return;
+
+ const Expr *Arg = Call->getArg(0)->IgnoreImpCasts();
+ // Arguments which are macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+ return;
+
+ // We first handle the cases of literal zero (both float and integer).
+ if (Result.Nodes.getNodeAs<Stmt>("zero")) {
+ diag(Call->getBeginLoc(),
+ "use ZeroDuration() for zero-length time intervals")
+ << FixItHint::CreateReplacement(Call->getSourceRange(),
+ "absl::ZeroDuration()");
+ return;
+ }
+
+ const auto *CallDecl = Result.Nodes.getNodeAs<FunctionDecl>("call_decl");
+ auto MaybeScale = GetScaleForFactory(CallDecl->getName());
+ if (!MaybeScale)
+ return;
+
+ DurationScale Scale = *MaybeScale;
+
+ // We next handle the cases of multication.
+ // We need to look at both operands, and consider the cases where a user
+ // is multiplying by something such as 1e-3.
+ const auto *MultBinOp = Result.Nodes.getNodeAs<BinaryOperator>("mult_binop");
+ if (MultBinOp) {
+ const Expr *Remainder;
+ llvm::Optional<DurationScale> NewScale;
+
+ // First check the LHS
+ const auto *IntLit = llvm::dyn_cast<IntegerLiteral>(MultBinOp->getLHS());
+ const auto *FloatLit = llvm::dyn_cast<FloatingLiteral>(MultBinOp->getLHS());
+ if (IntLit || FloatLit) {
+ NewScale = GetNewMultScale(Scale, GetValue(IntLit, FloatLit));
+ if (NewScale)
+ Remainder = MultBinOp->getRHS();
+ }
+
+ // If we weren't able to scale based on the LHS, check the RHS
+ if (!NewScale) {
+ IntLit = llvm::dyn_cast<IntegerLiteral>(MultBinOp->getRHS());
+ FloatLit = llvm::dyn_cast<FloatingLiteral>(MultBinOp->getRHS());
+ if (IntLit || FloatLit) {
+ NewScale = GetNewMultScale(Scale, GetValue(IntLit, FloatLit));
+ if (NewScale)
+ Remainder = MultBinOp->getLHS();
+ }
+ }
+
+ if (NewScale) {
+ assert(Remainder);
+ // We've found an appropriate scaling factor and the new scale, so output
+ // the relevant fix.
+ diag(Call->getBeginLoc(), "internal duration scaling can be removed")
+ << FixItHint::CreateReplacement(
+ Call->getSourceRange(),
+ (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
+ tooling::fixit::getText(*Remainder, *Result.Context) + ")")
+ .str());
+ }
+ return;
+ }
+
+ // We next handle division.
+ const auto *DivBinOp = Result.Nodes.getNodeAs<BinaryOperator>("div_binop");
+ if (DivBinOp) {
+ // For division, we only check the RHS.
+ const auto *FloatLit = llvm::dyn_cast<FloatingLiteral>(DivBinOp->getRHS());
+
+ llvm::Optional<DurationScale> NewScale =
+ GetNewDivScale(Scale, FloatLit->getValueAsApproximateDouble());
+ if (NewScale) {
+ const Expr *Remainder = DivBinOp->getLHS();
+
+ // We've found an appropriate scaling factor and the new scale, so output
+ // the relevant fix.
+ diag(Call->getBeginLoc(), "internal duration scaling can be removed")
+ << FixItHint::CreateReplacement(
+ Call->getSourceRange(),
+ (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
+ tooling::fixit::getText(*Remainder, *Result.Context) + ")")
+ .str());
+ }
+ }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/abseil/CMakeLists.txt
===================================================================
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -4,6 +4,7 @@
AbseilTidyModule.cpp
DurationDivisionCheck.cpp
DurationFactoryFloatCheck.cpp
+ DurationFactoryScaleCheck.cpp
FasterStrsplitDelimiterCheck.cpp
NoInternalDependenciesCheck.cpp
NoNamespaceCheck.cpp
Index: clang-tidy/abseil/AbseilTidyModule.cpp
===================================================================
--- clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tidy/abseil/AbseilTidyModule.cpp
@@ -12,6 +12,7 @@
#include "../ClangTidyModuleRegistry.h"
#include "DurationDivisionCheck.h"
#include "DurationFactoryFloatCheck.h"
+#include "DurationFactoryScaleCheck.h"
#include "FasterStrsplitDelimiterCheck.h"
#include "NoInternalDependenciesCheck.h"
#include "NoNamespaceCheck.h"
@@ -30,6 +31,8 @@
"abseil-duration-division");
CheckFactories.registerCheck<DurationFactoryFloatCheck>(
"abseil-duration-factory-float");
+ CheckFactories.registerCheck<DurationFactoryScaleCheck>(
+ "abseil-duration-factory-scale");
CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
"abseil-faster-strsplit-delimiter");
CheckFactories.registerCheck<NoInternalDependenciesCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
