[clang-tidy] Add the abseil-duration-comparison check

[hwright]

Summary

This check finds instances where Duration values are being converted to a numeric value in a comparison expression, and suggests that the conversion happen on the other side of the expression to a Duration. See documentation for examples.

This also shuffles some code around so that the new check may perform in sone step simplifications also caught by other checks.

[JonasToth]

Could you please upload the diff with full context? Especially the parts with refactoring are harder to judge if the code around is not visible. Thank you :)

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

I think this could be made a DenseMap with just the right amount of dense entries. 12 elements seems not too much to me.
Does the key-type need to be std::string, or could it be StringRef(or StringLiteral making everything constexpr if possible)?
Is there some strange stuff with dangling pointers or other issues going on?

68 ↗

(On Diff #174719)

The basis for this "we know" might change in the future if abs::Duration does things differently. Is adding stuff allowed in the absl:: space? (i am not fluent with the guarantees that it gives). You could maybe assert that the find is always correct, depending on absl guarantees.

[JonasToth]

clang-tidy/abseil/DurationComparisonCheck.cpp

50 ↗

(On Diff #174719)

This variable is a little hard to read. Could you make a little wrapper-struct instead of the tuple to make clear which element represents what?
Otherwise, why not std::pair?

• same DenseMap argument
• same StringRef/StringLiteral instead string consideration

78 ↗

(On Diff #174719)

Please no auto here

79 ↗

(On Diff #174719)

Please use const auto* to make it clear that this is a pointer

99 ↗

(On Diff #174719)

please no auto, you can ellide the braces

104 ↗

(On Diff #174719)

ellide braces

108 ↗

(On Diff #174719)

in LLVM the TODO does not contain a name for the author.

125 ↗

(On Diff #174719)

the list here is somewhat duplicated with the static members in the functions at the top. it would be best to merge them.
Not sure on how much constexpr is supported by the llvm-datastructures, but a constexpr DenseMap.keys() would be nice. Did you try something along this line?

151 ↗

(On Diff #174719)

Please follow the naming convention and don't use auto here.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #174719)

braces can be ellided here

clang-tidy/abseil/DurationRewriter.cpp

51 ↗

(On Diff #174719)

This is a implicit pointer-to-bool-conversion, isn't it? I think for readability purposes this should be made clear with a binary comparison.

61 ↗

(On Diff #174719)

const auto *

95 ↗

(On Diff #174719)

please no auto, braces

100 ↗

(On Diff #174719)

same

docs/clang-tidy/checks/abseil-duration-comparison.rst

10 ↗

(On Diff #174719)

'floating-pointer' -> 'floating-point'?

test/clang-tidy/abseil-duration-comparison.cpp

68 ↗

(On Diff #174719)

The test-cases miss macros and templates, please add reasonable cases for each of these categories.

89 ↗

(On Diff #174719)

What would happen for a type, that can implicitly convert to a duration or double/int.

struct MetaBenchmarkResults {
    int64_t RunID;
   absl::Duration D;

  operator absl::Duration() { return D; }
};

auto R = MetaBenchmarkResults { /* Foo */ };
bool WithinReason = Threshold < R;

What is the correct behaviour there? I think this should be diagnosed too.

161 ↗

(On Diff #174719)

could you please add a cases similiar to this:

if (some_condition && very_very_very_long_variable_name
     < SomeDuration) {
  // ...
}

[JonasToth]

test/clang-tidy/abseil-duration-comparison.cpp

89 ↗

(On Diff #174719)

Hmmmm, I fiddled around in godbolt, and i think there is nothing to trick around with conversions, as they wont compile.
If you find something along the lines, please tell me. But i guess this is a non-issue.

https://godbolt.org/z/gqhhHo

153 ↗

(On Diff #174719)

Please test the other casts you match on as well (especially c-style)

[Eugene.Zelenko]

docs/clang-tidy/checks/abseil-duration-comparison.rst

11 ↗

(On Diff #174719)

Please fix double space.

34 ↗

(On Diff #174719)

Please remove ..

[hwright]

Sorry it's taken so long to get all the feedback addressed!

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

Conceptually, this could easily be constexpr, but my compiler doesn't seem to want to build something which is called static constexpr llvm::DenseMap<llvm::StringRef, DurationScale>. It's chief complaint is that such a type has a non-trivial destructor. Am I using this correctly?

50 ↗

(On Diff #174719)

std::pair works here.

I'll defer the DenseMap and StringRef/StringLiteral changes until we determine if they are actually possible.

68 ↗

(On Diff #174719)

absl:: could add things. In this case, I'm very confident they won't, but I've still added the assert.

108 ↗

(On Diff #174719)

I just removed the TODO (this isn't a required part of the check).

125 ↗

(On Diff #174719)

I haven't tried that exact formulation, but given the above issue with DenseMap, I'm not sure it will work. Happy to try once we get it ironed out.

Another thing I've thought about is factoring the functionDecl matcher into a separate function, because I expect it to be reused. I haven't been able to deduce what type that function would return.

[JonasToth]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

I honestly never tried to make a constexpr DenseMap<> but it makes sense it is not possible, as DenseMap is involved with dynamic memory after all, which is not possible with constexpr (yet). So you were my test-bunny ;)

I did reread the Data-structures section in the LLVM manual, i totally forgot about StringMap that maps strings to values.
DenseMap is good when mapping small values to each other, as we do here (const char* -> int (whatever the enum deduces too)), which would fit.
StringMap does allocations for the strings, which we don't need.

constexpr aside my bet is that DenseMap fits this case the better, because we can lay every thing out and never touch it again. Maybe someone else has better arguments for the decision :)

Soooooo: could you please try static const llvm::DenseMap<llvm::StringRef, DurationScale>? :)
(should work in theory: https://godbolt.org/z/Qo7Nv4)

125 ↗

(On Diff #174719)

the type is probably clang::ast_matchers::internal::Matcher<FunctionDecl>. Not sure what the bind does with the type though.

If that is not the case you can make a nice error with int Bad = functionDecl(anything());, the type should be printed somewhere ;)
You could maybe create a lambda for that matcher or an AST_MATCHER(...) which some checks do as well. All variants are in use with clang-tidy.

clang-tidy/abseil/DurationRewriter.h

20 ↗

(On Diff #175379)

You could specify the underlying type to std::int8 directly, making the enum smaller and a more space-efficient DenseMap.

[hwright]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

static const llvm::DenseMap<llvm::StringRef, DurationScale> works here. :)

50 ↗

(On Diff #174719)

...but using DenseMap here doesn't work. From the mountains of compiler output I get when I try, it appears that DenseMap adds some constraints on the key type, and an enum class doesn't meet those constraints.

fwiw, the errors are mostly of this form:

/llvm/llvm/include/llvm/ADT/DenseMap.h:1277:45: error: ‘getEmptyKey’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
     const KeyT Empty = KeyInfoT::getEmptyKey();
                        ~~~~~~~~~~~~~~~~~~~~~^~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1278:53: error: ‘getTombstoneKey’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
     const KeyT Tombstone = KeyInfoT::getTombstoneKey();
                            ~~~~~~~~~~~~~~~~~~~~~~~~~^~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1280:44: error: ‘isEqual’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
     while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
                           ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1281:44: error: ‘isEqual’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
                           KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))

125 ↗

(On Diff #174719)

I was able to get both the factory matcher and the conversion matcher factored out. It's still a little messy, since the result has to be wrapped with a functionDecl at the call site, but that how the type system wanted it. (And it also allows for the bind call to work.)

[sammccall]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

Sorry for the drive-by...
This has a non-trivial destructor, so violates https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors at least in spirit.

Best to leak it (auto &ScaleMap = *new const llvm::DenseMap<...>(...)) to avoid the destructor issues. The constructor issues are already handled by making it function-local.

[aaron.ballman]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

I do not think this violates the coding standard -- that's talking mostly about global objects, not local objects, and is concerned about startup performance and initialization orders. I don't see any destructor ordering issues with this, so I do not think any of that applies here and leaking would be less than ideal.

[sammccall]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

There are three main issues with global objects that the coding standard mentions:

• performance when unused. Not relevant to function-local statics, only constructed when used
• static initialization order fiasco (constructors). Not relevant to function-local statics, constructed in well-defined order
• static initialization order fiasco (destructors). Just as relevant to function-local statics as to any other object!

That's why I say it violates the rule in spirit: the destructor is global. https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2

I don't see any destructor ordering issues with this

The reason these constructs are disallowed in coding guidelines is that humans can't see the problems, and they manifest in non-local ways :-(

leaking would be less than ideal

Why? The current code will (usually) destroy the object when the program exits. This is a waste of CPU, the OS will free the memory.

[aaron.ballman]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

The reason these constructs are disallowed in coding guidelines is that humans can't see the problems, and they manifest in non-local ways :-(

Can you explain why you think this would have any non-local impact on destruction? The DenseMap is local to the function and a pointer to it never escapes (so nothing can rely on it), and the data contained are StringRefs wrapping string literals and integer values.

Why? The current code will (usually) destroy the object when the program exits. This is a waste of CPU, the OS will free the memory.

Static analysis tools will start barking about memory leaks which then adds noise to the output, hiding real issues.

[sammccall]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

Can you explain why you think this would have any non-local impact on destruction?

I haven't analyzed this case in detail. A trivial example is running this check on a detached thread (sure, don't do that, but that's a non-local constraint...). It's possible there are no others here.

Static analysis tools will start barking about memory leaks which then adds noise to the output, hiding real issues.

This is a common pattern (from C++ FAQ) that tools are IME aware of. The object is still reachable so it's not truly a leak. (The heap checkers I know of are dynamic, but this is even easier to recognize statically).

(@hwright: sorry for the derail here, I know you're aware of this issue)

[JonasToth]

clang-tidy/abseil/DurationComparisonCheck.cpp

50 ↗

(On Diff #174719)

in order to use DenseMap you need to specialize the DenseMapInfo for types, that are not supported yet. More in the llvm manual and by examples.
Storing the integer is no issue, but the enum class makes its a type on its own, same with the pair.

I believe its fine to leave it an unordered_map, even though it might be slower on access. You can certainly do the extra-mile.

[JonasToth]

clang-tidy/abseil/DurationRewriter.h

62 ↗

(On Diff #175476)

I think you can even make this an AST_MATCHER(FunctionDecl, durationConversionFunction) { ... }, or was there an issue with it? (git grep -n AST_MATCHER in clang-tidy for other examples)
With this, the wrapping with functionDecl() should not be necessary.

[JonasToth]

clang-tidy/abseil/DurationRewriter.h

62 ↗

(On Diff #175476)

Nevermind, that was wrong. That would do functionDecl(durationConversionFunction()), sorry for noise.

[aaron.ballman]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

I guess I still read the coding standard differently. While it's certainly possible to have a finalization order fiasco with local statics, I believe you have to work considerably harder to craft one. I was doing some code archaeology on the words in the coding standard, and I don't think this was the scenario we had in mind when crafting that rule, but these words have been around for quite some time and I may have missed contextual discussion from the mailing lists.

It might make sense to propose a patch that adds some clarity to the coding standard in this area. We use static locals like this in numerous places (IdentifierNamingCheck.cpp, TypePromotionInMathFnCheck.cpp, TypeMismatchCheck.cpp, etc) and it would be good to nail down what we want to have happen in that case.

As for this code review, I still think using a heap allocation is not a good approach. If there is a serious concern about finalization ordering, we can use a ManagedStatic instead.

[hwright]

clang-tidy/abseil/DurationComparisonCheck.cpp

25 ↗

(On Diff #174719)

From all this discussion, it sounds like leaving the heap allocation as-is is what we want here.

Though I also understand an am sad about the potential for finalization ordering, and I'm doubly sad that C++ doesn't give us a good way to create a static map like this which doesn't require any finalization.

50 ↗

(On Diff #174719)

I'll just leave this as-in, since it looks like DenseMapInfo wants unused sentinel values, which I'm loathe to add to the enum.

[hwright]

Anything else for me here?

[JonasToth]

LG from my side, only the style nits left.
other reviewers are invited to take a look too :)

clang-tidy/abseil/DurationComparisonCheck.cpp

22 ↗

(On Diff #175476)

Please use triple / for the function comment, for doxygen and consistency with documentation

69 ↗

(On Diff #175476)

non-existence? Not sure about english, but i thought english does it that way

clang-tidy/abseil/DurationComparisonCheck.h

20 ↗

(On Diff #175476)

Missing /

clang-tidy/abseil/DurationDivisionCheck.h

23 ↗

(On Diff #175476)

I think that blank line could be removed, and it seems the comment is not ///, could you take a look at it too?
Touching this file is probably better to do in another patch anyway.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

The diagnostic is not printed if for some reason the fixit was not creatable. I think that the warning could still be emitted (auto Diag = diag(...); if (Fix) Diag << Fixit::-...)

clang-tidy/abseil/DurationRewriter.cpp

19 ↗

(On Diff #175476)

Comment

75 ↗

(On Diff #175476)

you can ellide the braces

85 ↗

(On Diff #175476)

you can ellide the braces here

clang-tidy/abseil/DurationRewriter.h

21 ↗

(On Diff #175476)

Same comment things in this file (///)

56 ↗

(On Diff #175476)

Double space

[hwright]

clang-tidy/abseil/DurationComparisonCheck.cpp

69 ↗

(On Diff #175476)

https://www.merriam-webster.com/dictionary/nonexistence tells me the hyphen isn't required.

clang-tidy/abseil/DurationDivisionCheck.h

23 ↗

(On Diff #175476)

Agreed. I think this snuck into the patch; I'll remove it.

(It would be good to just clang-format everything in this directory in a separate patch.)

The comment issue with /// seems to be a common problem; is clang-tidy/add_new_check.py not generating correct code?

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

I'm not sure under which conditions you'd expect this to be an issue. Could you give me an example?

My expectation is that if we don't get a value back in SimpleArg, we don't have anything to change, so there wouldn't be a warning to emit.

[JonasToth]

clang-tidy/abseil/DurationDivisionCheck.h

23 ↗

(On Diff #175476)

add_new_check does ///, maybe IDE settings or so removed these? Maybe someone created everything manually, dunno.

Doing the clang-format is ok, doesn't need review either (but plz run the test before committing to master).

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

I don't expect this to fail, failure there would mean a bug i guess. Having bugs is somewhat expected :)
And that would be our way to find the bug, because some user reports that there is no transformation done, that is my motivation behind that.

The warning itself should be correct, otherwise the matcher does not work, right? This would just be precaution.

[hwright]

clang-tidy/abseil/DurationDivisionCheck.h

23 ↗

(On Diff #175476)

I don't think I yet have the commit bit...so somebody else wouldn't need to directly commit. :)

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

I guess what I'm saying is that I'm not sure what kind of warning we'd give if we weren't able to offer a fix. The optionality here means that we found a result which was already as simple as we can make it, so there's no reason to bother the user.

[JonasToth]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

Lets say the result comes out of arithmetic and then there is a comparison (would be good test btw :)), the warning is still valueable, even if the transformation fails for some reason. The transformation could fail as well, because macros interfere or so. Past experience tells me, there are enough language constructs to break everything in weird combinations :)

[hwright]

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

I can buy that, but I'm still having trouble seeing the specifics. Do you have a concrete example?

[hwright]

Ping.

[hwright]

Slightly simplify the fixit text

[JonasToth]

Only the test and your opinion on the Optional thing, If you want to keep it that way its fine.

LGTM afterwards :)

clang-tidy/abseil/DurationFactoryFloatCheck.cpp

61 ↗

(On Diff #175476)

No i can't think of one right now. Its more a general argument, that the code-layout logically allows that only the transformation fails, even if the detection succeeds. No strong opinion, but preference :)

test/clang-tidy/abseil-duration-comparison.cpp

148 ↗

(On Diff #176103)

please add a test where the int part is result of a complex arithmetic expression.

[hwright]

Add additional test

[hwright]

@JonasToth reminder that you (or somebody else) will need to commit this for me.

[hwright]

Oh, and thanks for taking the time to review this. :)

[JonasToth]

In D54737#1314946, @hwright wrote:

Oh, and thanks for taking the time to review this. :)

No problem!
I will commit on Monday evening (Europe evening) if there are no further comments from other reviewers.
Thank you for the patch!

[JonasToth]

I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)

[riccibruno]

In D54737#1317128, @JonasToth wrote:

I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)

int8 ? Did you mean int8_t or am I missing somthing ?

[JonasToth]

int8 ? Did you mean int8_t or am I missing somthing ?

Your right, but the solution I wrote did actually not work anyway.. I just specialized std::hash<> now.

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 "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.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<DurationComparisonCheck>(
+        "abseil-duration-comparison");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");
     CheckFactories.registerCheck<DurationFactoryFloatCheck>(
         "abseil-duration-factory-float");
     CheckFactories.registerCheck<DurationFactoryScaleCheck>(
         "abseil-duration-factory-scale");
     CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
         "abseil-faster-strsplit-delimiter");

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

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

clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h

+//===--- DurationComparisonCheck.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_DURATIONCOMPARISONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Prefer comparison in the absl::Duration domain instead of the numeric
+/// domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-comparison.html
+class DurationComparisonCheck : public ClangTidyCheck {
+public:
+  DurationComparisonCheck(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_DURATIONCOMPARISONCHECK_H

clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp

+//===--- DurationComparisonCheck.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 "DurationComparisonCheck.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 {
+
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::DenseMap<llvm::StringRef, DurationScale> ScaleMap(
+      {{"ToDoubleHours", DurationScale::Hours},
+       {"ToInt64Hours", DurationScale::Hours},
+       {"ToDoubleMinutes", DurationScale::Minutes},
+       {"ToInt64Minutes", DurationScale::Minutes},
+       {"ToDoubleSeconds", DurationScale::Seconds},
+       {"ToInt64Seconds", DurationScale::Seconds},
+       {"ToDoubleMilliseconds", DurationScale::Milliseconds},
+       {"ToInt64Milliseconds", DurationScale::Milliseconds},
+       {"ToDoubleMicroseconds", DurationScale::Microseconds},
+       {"ToInt64Microseconds", DurationScale::Microseconds},
+       {"ToDoubleNanoseconds", DurationScale::Nanoseconds},
+       {"ToInt64Nanoseconds", DurationScale::Nanoseconds}});
+
+  auto ScaleIter = ScaleMap.find(std::string(Name));
+  if (ScaleIter == ScaleMap.end())
+    return llvm::None;
+
+  return ScaleIter->second;
+}
+
+/// Given a `Scale` return the inverse functions for it.
+static const std::pair<llvm::StringRef, llvm::StringRef> &
+getInverseForScale(DurationScale Scale) {
+  static const std::unordered_map<DurationScale,
+                                  std::pair<llvm::StringRef, llvm::StringRef>>
+      InverseMap(
+          {{DurationScale::Hours,
+            std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")},
+           {DurationScale::Minutes, std::make_pair("::absl::ToDoubleMinutes",
+                                                   "::absl::ToInt64Minutes")},
+           {DurationScale::Seconds, std::make_pair("::absl::ToDoubleSeconds",
+                                                   "::absl::ToInt64Seconds")},
+           {DurationScale::Milliseconds,
+            std::make_pair("::absl::ToDoubleMilliseconds",
+                           "::absl::ToInt64Milliseconds")},
+           {DurationScale::Microseconds,
+            std::make_pair("::absl::ToDoubleMicroseconds",
+                           "::absl::ToInt64Microseconds")},
+           {DurationScale::Nanoseconds,
+            std::make_pair("::absl::ToDoubleNanoseconds",
+                           "::absl::ToInt64Nanoseconds")}});
+
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.
+  auto InverseIter = InverseMap.find(Scale);
+  assert(InverseIter != InverseMap.end() && "Unexpected scale found");
+  return InverseIter->second;
+}
+
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+                                DurationScale Scale, const Expr &Node) {
+  const std::pair<std::string, std::string> &InverseFunctions =
+      getInverseForScale(Scale);
+  if (const Expr *MaybeCallArg = selectFirst<const Expr>(
+          "e", match(callExpr(callee(functionDecl(
+                                  hasAnyName(InverseFunctions.first.c_str(),
+                                             InverseFunctions.second.c_str()))),
+                              hasArgument(0, expr().bind("e"))),
+                     Node, *Result.Context))) {
+    return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }
+
+  return llvm::None;
+}
+
+/// Assuming `Node` has type `double` or `int` representing a time interval of
+/// `Scale`, return the expression to make it a suitable `Duration`.
+static llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+    const Expr *Node) {
+  const Expr &RootNode = *Node->IgnoreParenImpCasts();
+
+  if (RootNode.getBeginLoc().isMacroID())
+    return llvm::None;
+
+  // First check to see if we can undo a complimentary function call.
+  if (llvm::Optional<std::string> MaybeRewrite =
+          maybeRewriteInverseDurationCall(Result, Scale, RootNode))
+    return *MaybeRewrite;
+
+  if (IsLiteralZero(Result, RootNode))
+    return std::string("absl::ZeroDuration()");
+
+  return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
+          simplifyDurationFactoryArg(Result, RootNode) + ")")
+      .str();
+}
+
+void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher =
+      binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+                           hasOperatorName("=="), hasOperatorName("<="),
+                           hasOperatorName("<")),
+                     hasEitherOperand(ignoringImpCasts(callExpr(
+                         callee(functionDecl(DurationConversionFunction())
+                                    .bind("function_decl"))))))
+          .bind("binop");
+
+  Finder->addMatcher(Matcher, this);
+}
+
+void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+
+  // Don't try to replace things inside of macro definitions.
+  if (Binop->getExprLoc().isMacroID())
+    return;
+
+  llvm::Optional<DurationScale> Scale = getScaleForInverse(
+      Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
+  if (!Scale)
+    return;
+
+  // In most cases, we'll only need to rewrite one of the sides, but we also
+  // want to handle the case of rewriting both sides. This is much simpler if
+  // we unconditionally try and rewrite both, and let the rewriter determine
+  // if nothing needs to be done.
+  llvm::Optional<std::string> LhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
+  llvm::Optional<std::string> RhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
+
+  if (!(LhsReplacement && RhsReplacement))
+    return;
+
+  diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
+      << FixItHint::CreateReplacement(Binop->getSourceRange(),
+                                      (llvm::Twine(*LhsReplacement) + " " +
+                                       Binop->getOpcodeStr() + " " +
+                                       *RhsReplacement)
+                                          .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

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 "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 {
 
-// 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"))))
+      callExpr(callee(functionDecl(DurationFactoryFunction())),
+               hasArgument(0, anyOf(cxxStaticCastExpr(hasDestinationType(
+                                        realFloatingPointType())),
+                                    cStyleCastExpr(hasDestinationType(
+                                        realFloatingPointType())),
+                                    cxxFunctionalCastExpr(hasDestinationType(
+                                        realFloatingPointType())),
+                                    floatLiteral())))
           .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;
-  }
+  llvm::Optional<std::string> SimpleArg = stripFloatCast(Result, *Arg);
+  if (!SimpleArg)
+    SimpleArg = stripFloatLiteralFraction(Result, *Arg);
 
-  // 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;
-    }
+  if (SimpleArg) {
+        << FixItHint::CreateReplacement(Arg->getSourceRange(), *SimpleArg);
   }
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/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 "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 {
 
-namespace {
-
-// Potential scales of our inputs.
-enum class DurationScale {
-  Hours,
-  Minutes,
-  Seconds,
-  Milliseconds,
-  Microseconds,
-  Nanoseconds,
-};
-
-} // namespace
-
 // 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 std::unordered_map<std::string, DurationScale> ScaleMap(
       {{"Nanoseconds", DurationScale::Nanoseconds},
        {"Microseconds", DurationScale::Microseconds},
if (std::get<1>(*result) == 1.0)
       return std::get<0>(*result);
     Multiplier = std::get<1>(*result);
     OldScale = std::get<0>(*result);
   }
 
   return llvm::None;
 }
 
-// Given a `Scale`, return the appropriate factory function call for
-// constructing a `Duration` for that scale.
-static llvm::StringRef 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";
-  }
-  llvm_unreachable("unknown scaling factor");
-}
-
 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")),
+          callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
           hasArgument(
               0,
               ignoringImpCasts(anyOf(
-                  integerLiteral(equals(0)).bind("zero"),
-                  floatLiteral(equals(0.0)).bind("zero"),
+                  integerLiteral(equals(0)), floatLiteral(equals(0.0)),
                   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 to replace things inside of macro definitions.
   if (Call->getExprLoc().isMacroID())
     return;
 
   const Expr *Arg = Call->getArg(0)->IgnoreParenImpCasts();
   // Arguments which are macros are ignored.
   if (Arg->getBeginLoc().isMacroID())
     return;
 
   // We first handle the cases of literal zero (both float and integer).
-  if (Result.Nodes.getNodeAs<Stmt>("zero")) {
+  if (IsLiteralZero(Result, *Arg)) {
     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");
if (const auto *MultBinOp =
     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)) + "(" +
+                 (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
                   tooling::fixit::getText(*Remainder, *Result.Context) + ")")
                      .str());
     }
   }
 
   if (NewScale) {
     assert(Remainder && "No remainder found");
     // 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)) + "(" +
+               (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
                 tooling::fixit::getText(*Remainder, *Result.Context) + ")")
                    .str());
   }
   return;
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h

+//===--- DurationRewriter.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_DURATIONREWRITER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include <cinttypes>
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Duration factory and conversion scales
+enum class DurationScale : std::int8_t {
+  Hours,
+  Minutes,
+  Seconds,
+  Milliseconds,
+  Microseconds,
+  Nanoseconds,
+};
+
+/// Given a `Scale`, return the appropriate factory function call for
+/// constructing a `Duration` for that scale.
+llvm::StringRef getFactoryForScale(DurationScale Scale);
+
+// Determine if `Node` represents a literal floating point or integral zero.
+bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result,
+                   const Expr &Node);
+
+/// Possibly strip a floating point cast expression.
+///
+/// If `Node` represents an explicit cast to a floating point type, return
+/// the textual context of the cast argument, otherwise `None`.
+llvm::Optional<std::string>
+stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
+               const Expr &Node);
+
+/// Possibly remove the fractional part of a floating point literal.
+///
+/// If `Node` represents a floating point literal with a zero fractional part,
+/// return the textual context of the integral part, otherwise `None`.
+llvm::Optional<std::string>
+stripFloatLiteralFraction(const ast_matchers::MatchFinder::MatchResult &Result,
+                          const Expr &Node);
+
+/// Possibly further simplify a duration factory function's argument, without
+/// changing the scale of the factory function. Return that simplification or
+/// the text of the argument if no simplification is possible.
+std::string
+simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result,
+                           const Expr &Node);
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+                     DurationConversionFunction) {
+  using namespace clang::ast_matchers;
+  return functionDecl(
+      hasAnyName("::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+                 "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
+                 "::absl::ToDoubleMicroseconds", "::absl::ToDoubleNanoseconds",
+                 "::absl::ToInt64Hours", "::absl::ToInt64Minutes",
+                 "::absl::ToInt64Seconds", "::absl::ToInt64Milliseconds",
+                 "::absl::ToInt64Microseconds", "::absl::ToInt64Nanoseconds"));
+}
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+                     DurationFactoryFunction) {
+  using namespace clang::ast_matchers;
+  return functionDecl(hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
+                                 "::absl::Milliseconds", "::absl::Seconds",
+                                 "::absl::Minutes", "::absl::Hours"));
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H

clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp

+//===--- DurationRewriter.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 "DurationRewriter.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 the factory function name for a given `Scale`.
+llvm::StringRef 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";
+  }
+  llvm_unreachable("unknown scaling factor");
+}
+
+/// Returns `true` if `Node` is a value which evaluates to a literal `0`.
+bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) {
+  return selectFirst<const clang::Expr>(
+             "val",
+             match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)),
+                                               floatLiteral(equals(0.0)))))
+                       .bind("val"),
+                   Node, *Result.Context)) != nullptr;
+}
+
+llvm::Optional<std::string>
+stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
+               const Expr &Node) {
+  if (const Expr *MaybeCastArg = selectFirst<const Expr>(
+          "cast_arg",
+          match(expr(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"))))),
+                Node, *Result.Context)))
+    return tooling::fixit::getText(*MaybeCastArg, *Result.Context).str();
+
+  return llvm::None;
+}
+
+llvm::Optional<std::string>
+stripFloatLiteralFraction(const MatchFinder::MatchResult &Result,
+                          const Expr &Node) {
+  if (const auto *LitFloat = llvm::dyn_cast<FloatingLiteral>(&Node))
+    // Attempt to simplify a `Duration` factory call with a literal argument.
+    if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat))
+      return IntValue->toString(/*radix=*/10);
+
+  return llvm::None;
+}
+
+std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result,
+                                       const Expr &Node) {
+  // Check for an explicit cast to `float` or `double`.
+  if (llvm::Optional<std::string> MaybeArg = stripFloatCast(Result, Node))
+    return *MaybeArg;
+
+  // Check for floats without fractional components.
+  if (llvm::Optional<std::string> MaybeArg =
+          stripFloatLiteralFraction(Result, Node))
+    return *MaybeArg;
+
+  // We couldn't simplify any further, so return the argument text.
+  return tooling::fixit::getText(Node, *Result.Context).str();
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

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

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`abseil-duration-comparison
+  <clang-tidy/checks/abseil-duration-comparison>` check.
+
+  Checks for comparisons which should be done in the ``absl::Duration`` domain
+  instead of the float of integer domains.
+
 - 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-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst

+.. title:: clang-tidy - abseil-duration-comparison
+
+abseil-duration-comparison
+==========================
+
+Checks for comparisons which should be in the ``absl::Duration`` domain instead
+of the floating point or integer domains.
+
+N.B.: In cases where a ``Duration`` was being converted to an integer and then
+compared against a floating-point value, truncation during the ``Duration``
+conversion might yield a different result. In practice this is very rare, and
+still indicates a bug which should be fixed.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Comparison in the floating point domain
+  double x;
+  absl::Duration d;
+  if (x < absl::ToDoubleSeconds(d)) ...
+
+  // Suggested - Compare in the absl::Duration domain instead
+  if (absl::Seconds(x) < d) ...
+
+
+  // Original - Comparison in the integer domain
+  int x;
+  absl::Duration d;
+  if (x < absl::ToInt64Microseconds(d)) ...
+
+  // Suggested - Compare in the absl::Duration domain instead
+  if (absl::Microseconds(x) < d) ...

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

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-duration-comparison
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-string-find-startswith

clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp

+// RUN: %check_clang_tidy %s abseil-duration-comparison %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+class Time{};
+
+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
+
+using int64_t = long long int;
+
+double ToDoubleHours(Duration d);
+double ToDoubleMinutes(Duration d);
+double ToDoubleSeconds(Duration d);
+double ToDoubleMilliseconds(Duration d);
+double ToDoubleMicroseconds(Duration d);
+double ToDoubleNanoseconds(Duration d);
+int64_t ToInt64Hours(Duration d);
+int64_t ToInt64Minutes(Duration d);
+int64_t ToInt64Seconds(Duration d);
+int64_t ToInt64Milliseconds(Duration d);
+int64_t ToInt64Microseconds(Duration d);
+int64_t ToInt64Nanoseconds(Duration d);
+
+// Relational Operators
+constexpr bool operator<(Duration lhs, Duration rhs);
+constexpr bool operator>(Duration lhs, Duration rhs);
+constexpr bool operator>=(Duration lhs, Duration rhs);
+constexpr bool operator<=(Duration lhs, Duration rhs);
+constexpr bool operator==(Duration lhs, Duration rhs);
+constexpr bool operator!=(Duration lhs, Duration rhs);
+
+// Additive Operators
+inline Time operator+(Time lhs, Duration rhs);
+inline Time operator+(Duration lhs, Time rhs);
+inline Time operator-(Time lhs, Duration rhs);
+inline Duration operator-(Time lhs, Time rhs);
+
+}  // namespace absl
+
+void f() {
+  double x;
+  absl::Duration d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 <= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) == x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 == absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) >= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 >= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) > x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > absl::Seconds(x);
+
+  // Comparison against zero
+  b = absl::ToDoubleSeconds(d1) < 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::ZeroDuration();
+  b = absl::ToDoubleSeconds(d1) < 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::ZeroDuration();
+
+  // Scales other than Seconds
+  b = x > absl::ToDoubleMicroseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Microseconds(x) > d1;
+  b = x >= absl::ToDoubleMilliseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Milliseconds(x) >= d1;
+  b = x == absl::ToDoubleNanoseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Nanoseconds(x) == d1;
+  b = x <= absl::ToDoubleMinutes(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToDoubleHours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Hours(x) < d1;
+
+  // Integer comparisons
+  b = x > absl::ToInt64Microseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Microseconds(x) > d1;
+  b = x >= absl::ToInt64Milliseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Milliseconds(x) >= d1;
+  b = x == absl::ToInt64Nanoseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Nanoseconds(x) == d1;
+  b = x == absl::ToInt64Seconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToInt64Minutes(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToInt64Hours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Hours(x) < d1;
+
+  // Other abseil-duration checks folded into this one
+  b = static_cast<double>(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = double(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = float(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = ((double)5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = 5.0 > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+
+  // A long expression
+  bool some_condition;
+  int very_very_very_very_long_variable_name;
+  absl::Duration SomeDuration;
+  if (some_condition && very_very_very_very_long_variable_name
+     < absl::ToDoubleSeconds(SomeDuration)) {
+  // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: if (some_condition && absl::Seconds(very_very_very_very_long_variable_name) < SomeDuration) {
+    return;
+  }
+
+  // A complex expression
+  int y;
+  b = (y + 5) * 10 > absl::ToDoubleMilliseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1;
+
+  // These should not match
+  b = 6 < 4;
+
+#define TODOUBLE(x) absl::ToDoubleSeconds(x)
+  b = 5.0 > TODOUBLE(d1);
+#undef TODOUBLE
+#define THIRTY 30.0
+  b = THIRTY > absl::ToDoubleSeconds(d1);
+#undef THIRTY
+}