hwright updated this revision to Diff 170803.
hwright marked an inline comment as done.
hwright added a comment.
Address reviewer comments.
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %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 ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(6);
+
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+ d = absl::Seconds(0x3.4p1);
+
+ // Negative literals (we don't yet handle this case)
+ d = absl::Seconds(-3.0);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template <int N>
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast<double>(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast<float>(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds((double) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes((float) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds(double(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(float(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ // This should not be flagged
+ d = absl::Seconds(static_cast<int>(5.0));
+ d = absl::Seconds((int) 5.0);
+ d = absl::Seconds(int(5.0));
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -5,6 +5,7 @@
.. toctree::
abseil-duration-division
+ abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
Index: docs/clang-tidy/checks/abseil-duration-factory-float.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/abseil-duration-factory-float.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - abseil-duration-factory-float
+
+abseil-duration-factory-float
+=============================
+
+Checks for cases where the floating-point overloads of various
+``absl::Duration`` factory functions are called when the more-efficient
+integer versions could be used instead.
+
+This check will not suggest fixes for literals which contain fractional
+floating point values or non-literals. It will suggest removing
+superfluous casts.
+
+Examples:
+
+.. code-block:: c++
+
+ // Original - Providing a floating-point literal.
+ absl::Duration d = absl::Seconds(10.0);
+
+ // Suggested - Use an integer instead.
+ absl::Duration d = absl::Seconds(10);
+
+
+ // Original - Explicitly casting to a floating-point type.
+ absl::Duration d = absl::Seconds(static_cast<double>(10));
+
+ // Suggested - Remove the explicit cast
+ absl::Duration d = absl::Seconds(10);
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -74,6 +74,13 @@
floating-point context, and recommends the use of a function that
returns a floating-point value.
+- New :doc:`abseil-duration-factory-float
+ <clang-tidy/checks/abseil-duration-factory-float>` check.
+
+ Checks for cases where the floating-point overloads of various
+ ``absl::Duration`` factory functions are called when the more-efficient
+ integer versions could be used instead.
+
- New :doc:`abseil-faster-strsplit-delimiter
<clang-tidy/checks/abseil-faster-strsplit-delimiter>` check.
Index: clang-tidy/abseil/DurationFactoryFloatCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/abseil/DurationFactoryFloatCheck.h
@@ -0,0 +1,38 @@
+//===--- DurationFactoryFloatCheck.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_DURATIONFACTORYFLOATCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYFLOATCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check finds cases where `Duration` factories are being called with
+/// floating point arguments, but could be called using integer arguments.
+/// It handles explicit casts and floating point literals with no fractional
+/// component.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-factory-float.html
+class DurationFactoryFloatCheck : public ClangTidyCheck {
+public:
+ DurationFactoryFloatCheck(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_DURATIONFACTORYFLOATCHECK_H
Index: clang-tidy/abseil/DurationFactoryFloatCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/abseil/DurationFactoryFloatCheck.cpp
@@ -0,0 +1,106 @@
+//===--- DurationFactoryFloatCheck.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 "DurationFactoryFloatCheck.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 {
+
+// 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) {
+ return !clang::Lexer::makeFileCharRange(
+ clang::CharSourceRange::getCharRange(Range),
+ *Result.SourceManager, Result.Context->getLangOpts())
+ .isValid();
+}
+
+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"))))
+ .bind("call"),
+ this);
+}
+
+void DurationFactoryFloatCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedCall = Result.Nodes.getNodeAs<CallExpr>("call");
+
+ // Don't try and replace things inside of macro definitions.
+ if (InsideMacroDefinition(Result, MatchedCall->getSourceRange()))
+ return;
+
+ const Expr *Arg = MatchedCall->getArg(0)->IgnoreImpCasts();
+ // Arguments which are macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+ return;
+
+ // Check for casts to `float` or `double`.
+ if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) {
+ diag(MatchedCall->getBeginLoc(),
+ (llvm::Twine("Use 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 integer version of absl::") +
+ MatchedCall->getDirectCallee()->getName())
+ .str())
+ << FixItHint::CreateReplacement(LitFloat->getSourceRange(),
+ IntValue->toString(/*radix=*/10));
+ return;
+ }
+ }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/abseil/CMakeLists.txt
===================================================================
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_library(clangTidyAbseilModule
AbseilTidyModule.cpp
DurationDivisionCheck.cpp
+ DurationFactoryFloatCheck.cpp
FasterStrsplitDelimiterCheck.cpp
NoInternalDependenciesCheck.cpp
NoNamespaceCheck.cpp
Index: clang-tidy/abseil/AbseilTidyModule.cpp
===================================================================
--- clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tidy/abseil/AbseilTidyModule.cpp
@@ -11,6 +11,7 @@
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
#include "DurationDivisionCheck.h"
+#include "DurationFactoryFloatCheck.h"
#include "FasterStrsplitDelimiterCheck.h"
#include "NoInternalDependenciesCheck.h"
#include "NoNamespaceCheck.h"
@@ -27,6 +28,8 @@
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<DurationDivisionCheck>(
"abseil-duration-division");
+ CheckFactories.registerCheck<DurationFactoryFloatCheck>(
+ "abseil-duration-factory-float");
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
