hwright updated this revision to Diff 186569.
hwright marked 3 inline comments as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58137/new/
https://reviews.llvm.org/D58137
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationRewriter.cpp
clang-tidy/abseil/DurationRewriter.h
clang-tidy/abseil/TimeSubtractionCheck.cpp
clang-tidy/abseil/TimeSubtractionCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-time-subtraction.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-time-subtraction.cpp
Index: test/clang-tidy/abseil-time-subtraction.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/abseil-time-subtraction.cpp
@@ -0,0 +1,78 @@
+// RUN: %check_clang_tidy %s abseil-time-subtraction %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void g(absl::Duration d);
+
+void f() {
+ absl::Time t;
+ int x, y;
+ absl::Duration d;
+
+ d = absl::Hours(absl::ToUnixHours(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = (t - absl::FromUnixHours(x));
+ d = absl::Minutes(absl::ToUnixMinutes(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = (t - absl::FromUnixMinutes(x));
+ d = absl::Seconds(absl::ToUnixSeconds(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = (t - absl::FromUnixSeconds(x));
+ d = absl::Milliseconds(absl::ToUnixMillis(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = (t - absl::FromUnixMillis(x));
+ d = absl::Microseconds(absl::ToUnixMicros(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = (t - absl::FromUnixMicros(x));
+ d = absl::Nanoseconds(absl::ToUnixNanos(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = (t - absl::FromUnixNanos(x));
+
+ y = x - absl::ToUnixHours(t);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: y = absl::ToInt64Hours(absl::FromUnixHours(x) - t);
+ y = x - absl::ToUnixMinutes(t);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: y = absl::ToInt64Minutes(absl::FromUnixMinutes(x) - t);
+ y = x - absl::ToUnixSeconds(t);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: y = absl::ToInt64Seconds(absl::FromUnixSeconds(x) - t);
+ y = x - absl::ToUnixMillis(t);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: y = absl::ToInt64Milliseconds(absl::FromUnixMillis(x) - t);
+ y = x - absl::ToUnixMicros(t);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: y = absl::ToInt64Microseconds(absl::FromUnixMicros(x) - t);
+ y = x - absl::ToUnixNanos(t);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: y = absl::ToInt64Nanoseconds(absl::FromUnixNanos(x) - t);
+
+ // Check parenthesis placement
+ d = 5 * absl::Seconds(absl::ToUnixSeconds(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:11: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: d = 5 * (t - absl::FromUnixSeconds(x));
+
+ // No extra parens around arguments
+ g(absl::Seconds(absl::ToUnixSeconds(t) - x));
+ // CHECK-MESSAGES: [[@LINE-1]]:5: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: g(t - absl::FromUnixSeconds(x));
+ g(absl::Seconds(x - absl::ToUnixSeconds(t)));
+ // CHECK-MESSAGES: [[@LINE-1]]:5: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: g(absl::FromUnixSeconds(x) - t);
+
+ // These should not trigger; they are likely bugs
+ d = absl::Milliseconds(absl::ToUnixSeconds(t) - x);
+ d = absl::Seconds(absl::ToUnixMicros(t) - x);
+}
+
+absl::Duration parens_in_return() {
+ absl::Time t;
+ int x;
+
+ return absl::Seconds(absl::ToUnixSeconds(t) - x);
+ // CHECK-MESSAGES: [[@LINE-1]]:10: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: return t - absl::FromUnixSeconds(x);
+ return absl::Seconds(x - absl::ToUnixSeconds(t));
+ // CHECK-MESSAGES: [[@LINE-1]]:10: warning: perform subtraction in the time domain [abseil-time-subtraction]
+ // CHECK-FIXES: return absl::FromUnixSeconds(x) - t;
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -18,6 +18,7 @@
abseil-redundant-strcat-calls
abseil-str-cat-append
abseil-string-find-startswith
+ abseil-time-subtraction
abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-time-subtraction.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/abseil-time-subtraction.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - abseil-time-subtraction
+
+abseil-time-subtraction
+=======================
+
+Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
+in the Time domain instead of the numeric domain.
+
+There are two cases of Time subtraction in which deduce additional type
+information:
+ - When the result is an ``absl::Duration`` and the first argument is an
+ ``absl::Time``.
+ - When the second argument is a ``absl::Time``.
+
+In the first case, we must know the result of the operation, since without that
+the second operand could be either an ``absl::Time`` or an ``absl::Duration``.
+In the second case, the first operand *must* be an ``absl::Time``, because
+subtracting an ``absl::Time`` from an ``absl::Duration`` is not defined.
+
+Examples:
+
+.. code-block:: c++
+ int x;
+ absl::Time t;
+
+ // Original - absl::Duration result and first operand is a absl::Time.
+ absl::Duration d = absl::Seconds(absl::ToUnixSeconds(t) - x);
+
+ // Suggestion - Perform subtraction in the Time domain instead.
+ absl::Duration d = t - absl::FromUnixSeconds(x);
+
+
+ // Original - Second operand is an absl::Time.
+ int i = x - absl::ToUnixSeconds(t);
+
+ // Suggestion - Perform subtraction in the Time domain instead.
+ int i = absl::ToInt64Seconds(absl::FromUnixSeconds(x) - t);
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,12 @@
Finds and fixes cases where ``absl::Duration`` values are being converted to
numeric types and back again.
+- New :doc:`abseil-time-subtraction
+ <clang-tidy/checks/abseil-time-subtraction>` check.
+
+ Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
+ in the Time domain instead of the numeric domain.
+
- New :doc:`google-readability-avoid-underscore-in-googletest-name
<clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
check.
Index: clang-tidy/abseil/TimeSubtractionCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/abseil/TimeSubtractionCheck.h
@@ -0,0 +1,35 @@
+//===--- TimeSubtractionCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMESUBTRACTIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMESUBTRACTIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Finds and fixes `absl::Time` subtraction expressions to do subtraction
+/// in the Time domain instead of the numeric domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-time-subtraction.html
+class TimeSubtractionCheck : public ClangTidyCheck {
+public:
+ TimeSubtractionCheck(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_TIMESUBTRACTIONCHECK_H
Index: clang-tidy/abseil/TimeSubtractionCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/abseil/TimeSubtractionCheck.cpp
@@ -0,0 +1,163 @@
+//===--- TimeSubtractionCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TimeSubtractionCheck.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 {
+
+static bool isConstructorAssignment(const MatchFinder::MatchResult &Result,
+ const Expr *Node) {
+ return selectFirst<const Expr>(
+ "e", match(expr(hasParent(materializeTemporaryExpr(hasParent(
+ cxxConstructExpr(hasParent(exprWithCleanups(
+ hasParent(varDecl()))))))))
+ .bind("e"),
+ *Node, *Result.Context)) != nullptr;
+}
+
+static bool isArgument(const MatchFinder::MatchResult &Result,
+ const Expr *Node) {
+ return selectFirst<const Expr>(
+ "e",
+ match(expr(hasParent(
+ materializeTemporaryExpr(hasParent(cxxConstructExpr(
+ hasParent(callExpr()),
+ unless(hasParent(cxxOperatorCallExpr())))))))
+ .bind("e"),
+ *Node, *Result.Context)) != nullptr;
+}
+
+static bool isReturn(const MatchFinder::MatchResult &Result, const Expr *Node) {
+ return selectFirst<const Expr>(
+ "e", match(expr(hasParent(materializeTemporaryExpr(hasParent(
+ cxxConstructExpr(hasParent(exprWithCleanups(
+ hasParent(returnStmt()))))))))
+ .bind("e"),
+ *Node, *Result.Context)) != nullptr;
+}
+
+static bool parensRequired(const MatchFinder::MatchResult &Result,
+ const Expr *Node) {
+ // TODO: Figure out any more contexts in which we can omit the surrounding
+ // parentheses.
+ return !(isConstructorAssignment(Result, Node) || isArgument(Result, Node) ||
+ isReturn(Result, Node));
+}
+
+void TimeSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+ for (auto ScaleName :
+ {"Hours", "Minutes", "Seconds", "Millis", "Micros", "Nanos"}) {
+ std::string TimeInverse = (llvm::Twine("ToUnix") + ScaleName).str();
+ llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(TimeInverse);
+ if (!Scale)
+ continue;
+
+ auto TimeInverseMatcher = callExpr(callee(
+ functionDecl(hasName((llvm::Twine("::absl::") + TimeInverse).str()))
+ .bind("func_decl")));
+
+ // Match the cases where we know that the result is a Duration and the first
+ // argument is a Time. Just knowing the type of the first operand is not
+ // sufficient, since the second operand could be either a Time or a
+ // Duration. If we know the result is a Duration, we can then infer that
+ // the second operand must be a Time.
+ auto CallMatcher =
+ callExpr(
+ callee(functionDecl(hasName(getDurationFactoryForScale(*Scale)))),
+ hasArgument(0, binaryOperator(hasOperatorName("-"),
+ hasLHS(TimeInverseMatcher))
+ .bind("binop")))
+ .bind("outer_call");
+ Finder->addMatcher(CallMatcher, this);
+
+ // Match cases where we know the second operand is a Time. Since
+ // subtracting a Time from a Duration is not defined, in these cases, we
+ // always know the first operand is a Time if the second is a Time.
+ auto OperandMatcher =
+ binaryOperator(hasOperatorName("-"), hasRHS(TimeInverseMatcher))
+ .bind("binop");
+ Finder->addMatcher(OperandMatcher, this);
+ }
+}
+
+void TimeSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+ std::string inverse_name =
+ Result.Nodes.getNodeAs<FunctionDecl>("func_decl")->getNameAsString();
+
+ llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(inverse_name);
+ if (!Scale)
+ return;
+
+ const auto *OuterCall = Result.Nodes.getNodeAs<CallExpr>("outer_call");
+ if (OuterCall) {
+ // We're working with the first case of matcher, and need to replace the
+ // entire Duration factory call. (Which also means being careful about
+ // our order-of-operations and optionally putting in some parenthesis.
+ bool NeedParens = parensRequired(Result, OuterCall);
+
+ diag(OuterCall->getBeginLoc(), "perform subtraction in the time domain")
+ << FixItHint::CreateReplacement(
+ OuterCall->getSourceRange(),
+ (llvm::Twine(NeedParens ? "(" : "") +
+ rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) +
+ " - " +
+ rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) +
+ (NeedParens ? ")" : ""))
+ .str());
+ } else {
+ // We're working with the second case of matcher, and either just need to
+ // change the arguments, or perhaps remove an outer function call. In the
+ // latter case (addressed first), we also need to worry about parenthesis.
+ const auto *MaybeCallArg = selectFirst<const CallExpr>(
+ "arg", match(expr(hasAncestor(
+ callExpr(callee(functionDecl(hasName(
+ getDurationFactoryForScale(*Scale)))))
+ .bind("arg"))),
+ *BinOp, *Result.Context));
+ if (MaybeCallArg && MaybeCallArg->getArg(0)->IgnoreImpCasts() == BinOp) {
+ bool NeedParens = parensRequired(Result, MaybeCallArg);
+
+ diag(MaybeCallArg->getBeginLoc(),
+ "perform subtraction in the time domain")
+ << FixItHint::CreateReplacement(
+ MaybeCallArg->getSourceRange(),
+ (llvm::Twine(NeedParens ? "(" : "") +
+ rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) +
+ " - " +
+ rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) +
+ (NeedParens ? ")" : ""))
+ .str());
+ } else {
+ diag(BinOp->getBeginLoc(), "perform subtraction in the time domain")
+ << FixItHint::CreateReplacement(
+ BinOp->getSourceRange(),
+ (llvm::Twine(
+ getDurationInverseForScale(*Scale).second.str().substr(
+ 2)) +
+ "(" +
+ rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) +
+ " - " +
+ rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) +
+ ")")
+ .str());
+ }
+ }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/abseil/DurationRewriter.h
===================================================================
--- clang-tidy/abseil/DurationRewriter.h
+++ clang-tidy/abseil/DurationRewriter.h
@@ -31,6 +31,10 @@
/// constructing a `Duration` for that scale.
llvm::StringRef getDurationFactoryForScale(DurationScale Scale);
+/// Given a 'Scale', return the appropriate factory function call for
+/// constructing a `Time` for that scale.
+llvm::StringRef getTimeFactoryForScale(DurationScale scale);
+
// Determine if `Node` represents a literal floating point or integral zero.
bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result,
const Expr &Node);
@@ -81,6 +85,12 @@
const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
const Expr *Node);
+/// Assuming `Node` has a type `int` representing a time instant of `Scale`
+/// since The Epoch, return the expression to make it a suitable `Time`.
+std::string rewriteExprFromNumberToTime(
+ const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+ const Expr *Node);
+
/// Return `true` if `E` is a either: not a macro at all; or an argument to
/// one. In the both cases, we often want to do the transformation.
bool isNotInMacro(const ast_matchers::MatchFinder::MatchResult &Result,
Index: clang-tidy/abseil/DurationRewriter.cpp
===================================================================
--- clang-tidy/abseil/DurationRewriter.cpp
+++ clang-tidy/abseil/DurationRewriter.cpp
@@ -84,6 +84,22 @@
return llvm::None;
}
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+rewriteInverseTimeCall(const MatchFinder::MatchResult &Result,
+ DurationScale Scale, const Expr &Node) {
+ const llvm::StringRef &InverseFunction = getTimeInverseForScale(Scale);
+ if (const auto *MaybeCallArg = selectFirst<const Expr>(
+ "e", match(callExpr(callee(functionDecl(hasName(InverseFunction))),
+ hasArgument(0, expr().bind("e"))),
+ Node, *Result.Context))) {
+ return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+ }
+
+ return llvm::None;
+}
+
/// Returns the factory function name for a given `Scale`.
llvm::StringRef getDurationFactoryForScale(DurationScale Scale) {
switch (Scale) {
@@ -103,6 +119,24 @@
llvm_unreachable("unknown scaling factor");
}
+llvm::StringRef getTimeFactoryForScale(DurationScale Scale) {
+ switch (Scale) {
+ case DurationScale::Hours:
+ return "absl::FromUnixHours";
+ case DurationScale::Minutes:
+ return "absl::FromUnixMinutes";
+ case DurationScale::Seconds:
+ return "absl::FromUnixSeconds";
+ case DurationScale::Milliseconds:
+ return "absl::FromUnixMillis";
+ case DurationScale::Microseconds:
+ return "absl::FromUnixMicros";
+ case DurationScale::Nanoseconds:
+ return "absl::FromUnixNanos";
+ }
+ llvm_unreachable("unknown scaling factor");
+}
+
/// Returns the Time factory function name for a given `Scale`.
llvm::StringRef getTimeInverseForScale(DurationScale scale) {
switch (scale) {
@@ -250,6 +284,24 @@
.str();
}
+std::string rewriteExprFromNumberToTime(
+ const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+ const Expr *Node) {
+ const Expr &RootNode = *Node->IgnoreParenImpCasts();
+
+ // First check to see if we can undo a complimentary function call.
+ if (llvm::Optional<std::string> MaybeRewrite =
+ rewriteInverseTimeCall(Result, Scale, RootNode))
+ return *MaybeRewrite;
+
+ if (IsLiteralZero(Result, RootNode))
+ return std::string("absl::UnixEpoch()");
+
+ return (llvm::Twine(getTimeFactoryForScale(Scale)) + "(" +
+ tooling::fixit::getText(RootNode, *Result.Context) + ")")
+ .str();
+}
+
bool isNotInMacro(const MatchFinder::MatchResult &Result, const Expr *E) {
if (!E->getBeginLoc().isMacroID())
return true;
Index: clang-tidy/abseil/CMakeLists.txt
===================================================================
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -17,6 +17,7 @@
RedundantStrcatCallsCheck.cpp
StrCatAppendCheck.cpp
StringFindStartswithCheck.cpp
+ TimeSubtractionCheck.cpp
UpgradeDurationConversionsCheck.cpp
LINK_LIBS
Index: clang-tidy/abseil/AbseilTidyModule.cpp
===================================================================
--- clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tidy/abseil/AbseilTidyModule.cpp
@@ -23,6 +23,7 @@
#include "RedundantStrcatCallsCheck.h"
#include "StringFindStartswithCheck.h"
#include "StrCatAppendCheck.h"
+#include "TimeSubtractionCheck.h"
#include "UpgradeDurationConversionsCheck.h"
namespace clang {
@@ -59,6 +60,8 @@
"abseil-str-cat-append");
CheckFactories.registerCheck<StringFindStartswithCheck>(
"abseil-string-find-startswith");
+ CheckFactories.registerCheck<TimeSubtractionCheck>(
+ "abseil-time-subtraction");
CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
"abseil-upgrade-duration-conversions");
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
