[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

25 ↗

(On Diff #169887)

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

27 ↗

(On Diff #169887)

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

65–66 ↗

(On Diff #169887)

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

87 ↗

(On Diff #169887)

const auto * will be as readable and less verbose.

docs/ReleaseNotes.rst

73 ↗

(On Diff #169887)

Please remove the FIXME

[Eugene.Zelenko]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

24 ↗

(On Diff #169887)

Please use static keyword instead of anonymous namespace.

70 ↗

(On Diff #169887)

May be Macros are ignored will sound better?

docs/ReleaseNotes.rst

70 ↗

(On Diff #169887)

Please sort new check lists alphabetically.

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

8 ↗

(On Diff #169887)

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

27 ↗

(On Diff #169887)

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

34 ↗

(On Diff #169946)

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)

61 ↗

(On Diff #169946)

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

69 ↗

(On Diff #169946)

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

72 ↗

(On Diff #169946)

missing full stop.

84 ↗

(On Diff #169946)

missing full stop

docs/clang-tidy/checks/list.rst

12 ↗

(On Diff #169946)

spurious change

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

32 ↗

(On Diff #169946)

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

24 ↗

(On Diff #169946)

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

[hwright]

Addressed reviewer comments.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

34 ↗

(On Diff #169946)

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

69 ↗

(On Diff #169946)

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

32 ↗

(On Diff #169946)

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

69 ↗

(On Diff #169946)

Ok, I misunderstood the code then :)

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

32 ↗

(On Diff #169946)

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 ↗

(On Diff #170083)

What about c-style casts?

67 ↗

(On Diff #170083)

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

86 ↗

(On Diff #170083)

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 ↗

(On Diff #170083)

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 ↗

(On Diff #170083)

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 ↗

(On Diff #170083)

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

51 ↗

(On Diff #170659)

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.

99 ↗

(On Diff #170659)

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

51 ↗

(On Diff #170659)

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.

99 ↗

(On Diff #170659)

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

[aaron.ballman]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

29–31 ↗

(On Diff #170659)

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

36 ↗

(On Diff #170659)

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.

54 ↗

(On Diff #170659)

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

74 ↗

(On Diff #170659)

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

[hwright]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

36 ↗

(On Diff #170659)

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 ↗

(On Diff #170803)

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

94 ↗

(On Diff #170803)

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

[hwright]

Update diagnostic text

[hwright]

clang-tidy/abseil/AbseilTidyModule.cpp

32 ↗

(On Diff #170803)

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

94 ↗

(On Diff #170803)

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

94 ↗

(On Diff #170803)

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

84 ↗

(On Diff #169946)

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

84 ↗

(On Diff #169946)

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-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DurationDivisionCheck.h"
+#include "DurationFactoryFloatCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   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>(
         "abseil-no-internal-dependencies");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
     CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
         "abseil-redundant-strcat-calls");
     CheckFactories.registerCheck<StrCatAppendCheck>(

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationDivisionCheck.cpp
+  DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/trunk/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 {
+
+/// 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

clang-tools-extra/trunk/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;
+
+    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 the 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 the integer version of absl::") +
+            MatchedCall->getDirectCallee()->getName())
+               .str())
+          << FixItHint::CreateReplacement(LitFloat->getSourceRange(),
+                                          IntValue->toString(/*radix=*/10));
+      return;
+    }
+  }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

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

 
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   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.
 
   Finds instances of ``absl::StrSplit()`` or ``absl::MaxSplits()`` where the
   delimiter is a single character string literal and replaces with a character.
 
 - New :doc:`abseil-no-internal-dependencies
   <clang-tidy/checks/abseil-no-internal-dependencies>` check.

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

+.. 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);

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

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-division
+   abseil-duration-factory-float
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-string-find-startswith
    abseil-str-cat-append
    android-cloexec-accept
    android-cloexec-accept4

clang-tools-extra/trunk/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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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));
+}