[clang-tidy] Add the abseil-duration-factory-float check

[hwright]

Summary

This check finds cases where calls to an absl::Duration factory could use the more efficient integer overload.

For example:
// Original - Providing a floating-point literal.
absl::Duration d = absl::Seconds(10.0);

// Suggested - Use an integer instead.
absl::Duration d = absl::Seconds(10);

[alexfh]

LG In general, but see a few comments inline.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

26

Please use the LLVM style (functionName). See http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

28

Probably doesn't matter much, but would std::modf be more appropriate in this context?

66–67

Lexer::makeFileCharRange may be a better (and less strict) way to test whether we can safely replace a range.

88

const auto * will be as readable and less verbose.

docs/ReleaseNotes.rst

73

Please remove the FIXME

[Eugene.Zelenko]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

25

Please use static keyword instead of anonymous namespace.

71

May be Macros are ignored will sound better?

docs/ReleaseNotes.rst

70

Please sort new check lists alphabetically.

docs/clang-tidy/checks/abseil-duration-factory-float.rst

9

Please fix double space.

[Eugene.Zelenko]

By the word, why this check could not be generalized to any function/method which have floating-point and integer variants?

[hwright]

Addressed review comments

[hwright]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

28

I'm not actually sure. Since we're checking the remainder against 0, we don't need to also separately get the integral part, since if the conditional passes, we know the original value is the integral part. It would seem that std::modf just adds more complexity.

[JonasToth]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

35

Wouldn't it make more sense to use std::uint64_t instead to correspond to the line above? And where does the signedness difference come from? (/*isUnsigned=*/false)

62

That is worth a helper function taking a SourceRange as argument.

70

maybe assert instead, as your comment above suggests that macros are already filtered out?

73

missing full stop.

85

missing full stop

docs/clang-tidy/checks/list.rst

12

spurious change

test/clang-tidy/abseil-duration-factory-float.cpp

33

Could you also provide test cases with hexadecimal floating literals, which are C++17? The thousand-separators could be checked as well (dunno the correct term right now, but the 1'000'000 feature).
Please add test cases for negative literals, too.

[zturner]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

25

All variables (local, global, function parameter) use exactly same naming convention CamelCase.

[hwright]

Addressed reviewer comments.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

35

I don't remember where the signedness difference came from, so I've made this std::int64_t.

70

This is a different check than above.

In the first case, we want to be sure we aren't replacing cases inside of a macro, such as:

#define SECONDS(x) absl::Seconds(x)
SECONDS(1.0)

In this one, we want to avoid changing the argument if it is itself a macro:

#define VAL 1.0
absl::Seconds(VAL);

So it is a separate check, not just a re-assertion of the first one.

test/clang-tidy/abseil-duration-factory-float.cpp

33

Added the hexadecimal floating literal tests.

The testing infrastructure wouldn't build the test source with 3'000 as a literal argument. (There's also an argument that by the time we get to the AST, that distinction is gone anyway and this test isn't the place to check comprehensive literal parsing.)

I've also added a negative literal test, to illustrate that we don't yet handle that case (which is in practice pretty rare). I'd rather add it in a separate change.

[JonasToth]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

70

Ok, I misunderstood the code then :)

test/clang-tidy/abseil-duration-factory-float.cpp

33

I am happy with that. I agree that parsing should deal with it fine and your code seems to do it fine as well. My experience is, that sometimes there are still surprises :)

[hwright]

Ping.

What are the next steps here?

[JonasToth]

In D53339#1269998, @hwright wrote:

Ping.

What are the next steps here?

Please mark all comments you consider resolved as 'Done', i think alex already kinda accepted it, but given there were more comments he should take another look.

[JonasToth]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

51

What about c-style casts?

67

Please clarfiy this comment a bit more, like Marcros as arguments are ignored. or the like.

86

please highlight the code construct ('Duration' in this case) here and in other comments to clarify its about the class in user-code.

clang-tidy/abseil/DurationFactoryFloatCheck.h

19

Please add more to the doc here, like This check finds ... and transforms these calls into ... or similar.

docs/clang-tidy/checks/abseil-duration-factory-float.rst

6

Please synchronize the first paragraph with the release notes (I would prefer the release notes version)

[hwright]

Address reviewer comments

clang-tidy/abseil/DurationFactoryFloatCheck.h

19

I've added some more text.

I'd prefer not to make this duplicate much of what is already in the user-facing documentation, since that should be more complete and canonical (and is already included by reference).

[JonasToth]

@alexfh you did comment before, do you want to add more? I have no issues left.

Please give alex the opportunity to react, but if he doesn't (he has a lot to do) you can commit in 3 days or so. Do you have commit access?

EDIT: did you run this check over abseil code? Did you notice anything unregular?

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

52

Nit: the duplication in the cast matcher can be removed with a variable auto CastToFloat = hasDestinationType(realFloatingPointType()), hasSourceExpression(expr().bind("cast_arg")); and then used in the matcher.

100

is it logically valid to fall reach the end of the method?
If not please add an llvm_unreachable()

[hwright]

I do not have commit privileges, so somebody else would need to submit it.

We've had a version of this check running over our internal codebase for several months with no unexpected problems.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

52

This turns out to be harder than it looks: auto can't be used here because it would be ambiguous, and the actual type is part of the internal namespace so I'm hesitant to use it here.

I've left the duplication as-is; if there's a better way to de-dupe, I'm happy to try it.

100

It is. Any case we don't catch would fall through to the end of this function.

[aaron.ballman]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

30–32

I believe you can do return APSInt::get(static_cast<int64_t>(Value)); instead -- it should do the same thing.

37

This function name doesn't seem to relate to the behavior of the function? Rather than try to pin it down like that, perhaps rename to "CanRangeBeSafelyReplaced()` and add a comment as to why this is the correct approach for answering the question.

55

What about function-style casts? e.g. float(1)

75

Comment is now out of date, this checks for a few kinds of casts.

[hwright]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

37

I've added a comment describing what the function does, but leaving the name the same.

Since this isn't the only reason why we reject a given range, and I think keeping those decisions co-located is probably best, I'd like to leave that logic in the check method below, rather than moving part of it up here.

[hwright]

Address reviewer comments.

[hokein]

looks good, just a few nits.

clang-tidy/abseil/AbseilTidyModule.cpp

32

Maybe drop the factory? we already have a duration-related check abseil-duration-division, for consistency.

clang-tidy/rename_check.py may help you.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

95

nit: clang-tidy message is not a complete sentence, use lower letter use.

[hwright]

Update diagnostic text

[hwright]

clang-tidy/abseil/AbseilTidyModule.cpp

32

The expectation is that there may be other checks relating to calls to duration factories, beyond just floating point literal and cast simplification.

Though I'm happy to be convinced otherwise.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

95

This is a complete sentence, in the imperative style of other clang-tidy checks. I've added a full-stop at the end.

[hokein]

LGTM.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

95

Ah, sorry for not clarifying it clearly. In clang-tidy, we don't use complete sentence for warnings, so just remove the "." at the end.

[alexfh]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

85

Clang-tidy (and clang) diagnostics don't end with a period. This one doesn't need to be special in this regard. Same below.

[JonasToth]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

85

I believe that was related to a comment to make it a full sentence with grammar and so on, AFAIK these are supposed to be correct.
The diagnostic message itself not, I did not intend to include it.

[hwright]

Remove full-stop

[JonasToth]

I think accepted now? :)
If you want I can commit for you and monitor the buildbot, if there are bigger problems I would come back to you.

[hwright]

In D53339#1274478, @JonasToth wrote:

I think accepted now? :)
If you want I can commit for you and monitor the buildbot, if there are bigger problems I would come back to you.

@JonasToth I don't actually have commit privileges, so somebody else will have to commit for me. :)

[JonasToth]

Thanks for the patch!

[alexfh]

In D53339#1274561, @hwright wrote:

@JonasToth I don't actually have commit privileges, so somebody else will have to commit for me. :)

You should definitely ask commit access.

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DurationDivisionCheck.h"
+#include "DurationFactoryFloatCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"

   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>(

clang-tidy/abseil/CMakeLists.txt

 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationDivisionCheck.cpp
+  DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

clang-tidy/abseil/DurationFactoryFloatCheck.h

+//===--- 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 {
+
+/// Prefer integer Duration factories when possible.
+///
+/// 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

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

+//===--- 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;
+
+    llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+    ApInt = static_cast<std::int64_t>(Value);
+    return ApInt;
+  }
+  return llvm::None;
+}
+
+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"))),
+                            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();
+  // Macros are ignored.
+  if (Arg->getBeginLoc().isMacroID())
+    return;
+
+  // Check for static casts to float.
+  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

docs/ReleaseNotes.rst

   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 is called when the more-efficient
+  integer version could be used instead.
+
 - New :doc:`abseil-faster-strsplit-delimiter
   <clang-tidy/checks/abseil-faster-strsplit-delimiter>` check.
 

docs/clang-tidy/checks/abseil-duration-factory-float.rst

+.. title:: clang-tidy - abseil-duration-factory-float
+
+abseil-duration-factory-float
+=============================
+
+Finds cases where callers of ``absl::Duration`` factory functions (such as
+``absl::Seconds`` or ``absl::Hours``) are providing a floating point value
+when an integer could be used instead. The integer factory function overload
+is more efficient and readable.
+
+This check will not suggest fixes for fractional floating point values or
+non-literals.
+
+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);

docs/clang-tidy/checks/list.rst

 
 .. toctree::
    abseil-duration-division
+   abseil-duration-factory-float
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace

test/clang-tidy/abseil-duration-factory-float.cpp

+// 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 ConvertStaticCastTest() {
+  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);
+
+  // This should not be flagged
+  d = absl::Seconds(static_cast<int>(5.0));
+}