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

[hwright]

Summary

This check removes unneeded scaling of arguments when calling Abseil Time factory functions.

[Eugene.Zelenko]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

36

This will cause memory leaks, so may be unique_ptr should be used to hold pointer? May be LLVM ADT has better container?

59

else after return. Same in other places.

164

Should be llvm_unreachable().

288

Please remove trailing return and comment above. See readability-redundant-control-flow.

docs/ReleaseNotes.rst

88

Second statement belongs to documentation.

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

7

Please fix double space.

[hwright]

Address reviewer comments

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

36

This is a tradeoff between leaking a small amount of known memory for the duration of the program, and constructing this map every time this function is invoked. Since I expect the latter to occur frequently, that's a tradeoff I think is acceptable. (Ideally, this would be a compile-time constant, but sadly we don't yet have a constexpr dictionary type.)

Of course, if there is a more typical way of doing that here, I'm happy to use it.

docs/ReleaseNotes.rst

88

It already is in the user-facing documentation, are saying it should be removed from here?

[Eugene.Zelenko]

docs/ReleaseNotes.rst

88

Release Notes are for brief descriptions. Documentation is for details.

[aaron.ballman]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

36

Why did you not use a static value type?

[hokein]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

23

nit: wrap it in anonymous namespace?

36

I think main reason is lazy initialization.

75

we are comparing with floats, I think we should use something AlmostEquals(Multiplier, 60.0).

203

isn't Call->getExprLoc().isMacroID() enough?

267

is this for debugging?

[aaron.ballman]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

36

Static locals are lazily initialized, so I'm still not certain why this shouldn't use static const std::unorderd_map<std::string, DurationScale> ScaleMap{...}; instead.

47

Elide braces.

58

Elide braces.

61

Please add a message to the assertion. e.g., assert(foo && "bar");

180–182

::absl instead -- otherwise this can trigger in unintended places.

[hwright]

Addressed reviewer feedback.

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

75

I can't find AlmostEquals elsewhere; could you point me to it so I can investigate it's use?

But I am also curious why you think we should use this. We are checking for exact values. 60.0 is representable exactly as a double, and even values which aren't (e.g., 1e-3) will have the same representation in this function as they do in the code being transformed, so equality is still appropriate.

203

Thanks! I've also added tests to verify this.

[aaron.ballman]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

40

GetScaleForFactory -> getScaleForFactory, per naming conventions. Same for the other functions.

49

Drop top-level const (this returns an iterator, not a pointer/reference).

56

it's -> its

58–59

I really don't like this interface where you pass two arguments, only one of which is ever valid. That is pretty confusing. Given that the result of this function is only ever passed to GetNewMultScale(), and that function only does integral checks, I'd prefer logic more like:

• If the literal is integral, get its value and call GetNewMultScale().
• If the literal is float, convert it to an integral and call GetNewMultScale() only if the conversion is exact (this can be done via APFloat::convertToInteger()).
• GetNewMultScale() can now accept an integer value and removes the questions about inexact equality tests from the function.

With that logic, I don't see a need for GetValue() at all, but if a helper function is useful, I'd probably guess this is a better signature: int64_t getIntegralValue(const Expr *Literal, bool &ResultIsExact);

60

Elide these braces.

64

I believe the approximate results here can lead to bugs where the floating-point literal is subnormal -- it may return 0.0 for literals that are not zero.

73

Hours with a multiplier of 1.0/60.0 would scale to minutes, wouldn't it?

82–85

What about scaling with a multiplier of 3600 to go from seconds to hours, and other plausible conversions?

113

Similar comments here as above. In fact, I feel like these functions should be combined into getNewScale() and have normalized the scaling value in the caller so that it covers both operations rather than manually spelling out the conversions in either direction. e.g., convert everything into multiplier form in the caller by treating divisors as a multiplication by 1/divisor.

151

This should return a StringRef.

195

and -> to

199

I suspect you want to ignore paren expressions as well, so IgnoreParenImpCasts().

214

Do not use auto here as the type is not explicitly spelled out in the initialization.

clang-tidy/abseil/DurationFactoryScaleCheck.h

20

funciton -> function

[hwright]

Addressed small concerns.

Larger issues pending feedback.

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

58–59

Given that the result of this function is only ever passed to GetNewMultScale(), and that function only does integral checks, I'd prefer logic more like:

That's actually not true: GetNewMultScale() does checks against values like 1e-3 which aren't integers. Does this change your suggestion?

64

Do you have an example which I could put in a test?

82–85

That's a good point, and part of a broader design discussion: should we support all multipliers? (e.g., what about multiplying microseconds by 1.0/86400000000.0?)

If we do think it's worth handling all of these cases, we probably want a different construct than the equivalent of a lookup table to do this computation.

[aaron.ballman]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

58–59

Hmm, yeah, I suppose it has to! :-D

64

0x0.000001p-126f should get you a new, exciting way to spell 0.

82–85

That's a good point, and part of a broader design discussion: should we support all multipliers?

That's kind of what I'm leaning towards. It's certainly more explainable to users that all the various scaling operations just work, rather than some particular set.

However, maybe you know more about the user base than I do and there's a sound reason to not handle all cases?

If we do think it's worth handling all of these cases, we probably want a different construct than the equivalent of a lookup table to do this computation.

There's a part of me that wonders if we can use std::ratio to describe the scaling operations, but I freely admit I've not thought about implementation strategies all that hard.

[hwright]

Combined multiplication and division logic, and also now handles scaling of multiple steps (e.g., Seconds * 3600).

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

58–59

I've reworked the bulk of this logic. It still uses doubles, but that doesn't trouble our test cases. Please let me know if there's more to do here.

64

I've added this as a test, and it resolves as normal. Was your comment intended to indicate that it should, or that doing so would be a bug?

82–85

std::ratio didn't look like it made much sense here (though I don't have much first-hand experience using it), but we do now handle multiple scaling steps.

[aaron.ballman]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

64

Hmm, in hindsight, I think this code is okay as-is. The situation I was worried about is where compile-time evaluation gives you one value (say, 0) while runtime evaluation gives you another (nonzero) because of the floating-point format for the target architecture. However, 1) I think APFloat does a good job of avoiding that, so we may be fine by default, and 2) I am questioning whether abseil users would be writing those constants anyway.

74

This doesn't seem quite right. 1.0/6000.0 is less than 1.0/60.0 but isn't a Minutes scaling.

[hwright]

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

74

After spending two days chasing this down, it finally occurred to me: we don't actually handle this case. Our (somewhat conservative) matchers only look for literal values, not division expressions like 1.0/60.0.

And for this iteration of the tool, I think I'm fine with that, since I expect those kinds of things to be exceedingly rare, and probably tricky to handle, anyway.

[hwright]

I think this is ready to go, please advise on next steps.

[aaron.ballman]

LGTM!

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

74

Oh jeez -- good point on not covering constant expressions from the matcher. If you think this is going to be exceedingly rare, I'm okay with punting until we need it. Thank you for looking into it!

[hwright]

@aaron.ballman I don't actually have the commit bit, can you commit this, or are we waiting for further review?

[aaron.ballman]

In D54246#1301940, @hwright wrote:

@aaron.ballman I don't actually have the commit bit, can you commit this, or are we waiting for further review?

No further review required. I'm happy to commit for you. I'll do it later today or tomorrow, unless someone else gets to it first.

[aaron.ballman]

I've commit in r347163. Thank you for the patch!

clang-tidy/abseil/AbseilTidyModule.cpp

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

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

clang-tidy/abseil/CMakeLists.txt

   AbseilTidyModule.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
+  DurationFactoryScaleCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

clang-tidy/abseil/DurationFactoryScaleCheck.h

+//===--- 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

clang-tidy/abseil/DurationFactoryScaleCheck.cpp

+//===--- 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 {
+
+// Potential scales of our inputs.
+enum class DurationScale {
+  Hours,
+  Minutes,
+  Seconds,
+  Milliseconds,
+  Microseconds,
+  Nanoseconds,
+};
+
+// 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 auto *ScaleMap =
+      new std::unordered_map<std::string, DurationScale>(
+          {{"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();
+  } else {
+    assert(FloatLit != nullptr);
+    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;
+    else if (Multiplier == 1e-3)
+      return DurationScale::Milliseconds;
+    break;
+
+  case DurationScale::Milliseconds:
+    if (Multiplier == 1e3)
+      return DurationScale::Seconds;
+    else if (Multiplier == 1e-3)
+      return DurationScale::Microseconds;
+    break;
+
+  case DurationScale::Microseconds:
+    if (Multiplier == 1e3)
+      return DurationScale::Milliseconds;
+    else 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";
+  }
+  return "unreachable";
+}
+
+// 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 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 (InsideMacroDefinition(Result, Call->getSourceRange()))
+    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) {
+    DivBinOp->dumpColor();
+    // 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());
+    }
+  }
+
+  // We can't do anything with this expression, so return.
+  return;
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

   ``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.  This
+  check also looks for arguements with a zero value and suggests using
+  ``absl::ZeroDuration()`` instead.
+
 - New :doc:`abseil-faster-strsplit-delimiter
   <clang-tidy/checks/abseil-faster-strsplit-delimiter>` check.
 

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

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

docs/clang-tidy/checks/list.rst

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

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

+// 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);
+}