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

[hwright]

Summary

foreach ($list as $item) {
  work_miracles($item);
}

This check uses the context of a subtraction expression as well as knowledge about the Abseil Time types, to infer the type of the second operand of some subtraction expressions in Duration conversions. For example:

absl::ToDoubleSeconds(duration) - foo

can become

absl::ToDoubleSeconds(duration - absl::Seconds(foo))

This ensures that time calculations are done in the proper domain, and also makes it easier to further deduce the types of the second operands to these expressions.

[hwright]

Fix docs.

[Eugene.Zelenko]

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

7 ↗

(On Diff #176536)

Please fix double space.

[hwright]

Fix double space.

[hwright]

Ping.

I assume I've got the right reviewers here, but I've also been sending a bunch of stuff your way lately, so if I'm overwhelming review capacity, please let me know.

[JonasToth]

In D55245#1320546, @hwright wrote:

Ping.

I assume I've got the right reviewers here, but I've also been sending a bunch of stuff your way lately, so if I'm overwhelming review capacity, please let me know.

Hi hyrum, you have the right reviews. I do not have review capacity today, but at the end of the week. We usually ping after a few days (~1 week), e.g. for me its a hobby and I need to work :)
If you have more patches in the pipeline, you can already work on them/push them to review. It is easier to review a bit more in one piece and generates higher throughput too.

If other reviewers have time, go for it :)

[hokein]

The check looks good from my side, except one nit.

I assume I've got the right reviewers here, but I've also been sending a bunch of stuff your way lately, so if I'm overwhelming review capacity, please let me know.

I think in long run, it would be desired to have Abseil experts participate in reviews of abseil-related checks (upstream clang-tidy reviewers don't have much knowledge about Abseil). Abseil team members (@astrelni, @ahedberg) might be more interested in theses checks which could accelerate the review process.

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

4 ↗

(On Diff #176757)

We have multiple implementations in other abseil-duration-* tests, I think we could pull them out (in test/clang-tidy/Inputs/absl/time/time.h).

[lebedev.ri]

Please always upload all patches with the full context (-U99999)

clang-tidy/abseil/DurationRewriter.cpp

35 ↗

(On Diff #176757)

https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options

We never use hash_set and unordered_set because they are generally very expensive (each insertion requires a malloc) and very non-portable.

Since the key is an enum, [[ https://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h | llvm/ADT/IndexedMap.h ]] should be a much better fit.

68 ↗

(On Diff #176757)

if (const auto *MaybeCallArg

68 ↗

(On Diff #176757)

Early return?

74 ↗

(On Diff #176757)

So you generate fix-it, and then immediately degrade it into a string. Weird.

[hwright]

clang-tidy/abseil/DurationRewriter.cpp

35 ↗

(On Diff #176757)

It doesn't look like IndexedMap has a constructor which takes an initializer list. Without it, this code get a bit more difficult to read, and I'd prefer to optimize for readability here.

68 ↗

(On Diff #176757)

I'm not quite sure what you mean by Early return? Are you suggesting that the call to selectFirst should be pulled out of the if conditional, and then the inverse checked to return llvm::None first?

74 ↗

(On Diff #176757)

This doesn't generate a fix-it, it just fetches the text of the given node as a StringRef but we're returning a string, so we need to convert.

Is there a more canonical method I should use to fetch a node's text?

[lebedev.ri]

clang-tidy/abseil/DurationRewriter.cpp

156 ↗

(On Diff #177266)

Are you very sure this shouldn't be [[ https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h | StringMap ]]?

35 ↗

(On Diff #176757)

The manual still 'recommends' not to use them.
Simple solution: immediately invoked lambda
Better solution: try to add such constructor to IndexedMap.

68 ↗

(On Diff #176757)

Ah, nevermind then.

74 ↗

(On Diff #176757)

I don't know the answer, but have you tried looking what tooling::fixit::getText() does internally?

[hwright]

Use an IndexedMap instead of an std::unordered_map

clang-tidy/abseil/DurationRewriter.cpp

156 ↗

(On Diff #177266)

Nope. Thanks for the catch!

35 ↗

(On Diff #176757)

In hopes of not making this too much of a yak shave, I've gone with the immediately invoked lambda.

74 ↗

(On Diff #176757)

I have. It calls internal::getText, which, from the namespace, I'm hesitant to call in this context.

[hwright]

Reminder: I'll need somebody to submit this for me, since I don't have subversion access.

[lebedev.ri]

clang-tidy/abseil/DurationRewriter.cpp

20–39 ↗

(On Diff #177325)

You should not need this if you change the enum instead.

66–77 ↗

(On Diff #177325)

I was thinking of more like

for(std::pair<??, ??> e : std::initializer_list({
                           {DurationScale::Hours, std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")}.
                           }))
  InverseMap[e.first] = e.second;

clang-tidy/abseil/DurationRewriter.h

22–23 ↗

(On Diff #177325)

enum class DurationScale : std::int8_t {
  Hours = 0,
  Minutes,
...

[hwright]

Use static_cast instead of a switch for IndexedMap lookup.

clang-tidy/abseil/DurationRewriter.cpp

20–39 ↗

(On Diff #177325)

The function is still required; the switch can be removed with a static_cast.

66–77 ↗

(On Diff #177325)

The compilable construction looks something like:

for (const auto &e : {std::make_pair(DurationScale::Hours,
                                    std::make_pair("::absl::ToDoubleHours",
                                                   "::absl::ToInt64Hours"))})

Which is a bit more verbose than just assigning values to the map (and not any more efficient), so I've just left this bit as-is.

[JonasToth]

clang-tidy/abseil/DurationRewriter.cpp

21 ↗

(On Diff #177509)

Are you using argument_type? Browser searching did only show one result.

23 ↗

(On Diff #177509)

Why not std::uint8_t as its the underlying type for the enum?

73 ↗

(On Diff #177509)

maybe in the name is redundant, as its return type is Optional

77 ↗

(On Diff #177509)

In Principle the Node could have multiple expressions that are a call if there is nesting.
The transformation is correct from what I see right now, but might result in the necessity of multiple passes for the check. (Is the addition version affected from that too?)

Given the recursive nature of the matcher you could construct a nesting with the inner part being a subtraction, too. The generated fixes would conflict and none of them would be applied. At least thats what I would expect right now. Please take a look at this issue.

test/clang-tidy/Inputs/absl/time/time.h

1 ↗

(On Diff #177509)

I think having the extraction of the common test-stuff into this header as one commit would be better. Would you prepare such a patch? I can commit for you. It probably makes sense if you ask for commit access (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Do as you wish.

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

12 ↗

(On Diff #177509)

From this example starting:

• The RHS should be a nested expression with function calls, as the RHS is transformed to create the adversary example i mean in the transformation function above.

absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1));

I think you need the proper conversion function, as the result of the expression is double and you need a Duration, right?

But in principle starting from this idea the transformation might break.

[hwright]

Add tests

clang-tidy/abseil/DurationRewriter.cpp

21 ↗

(On Diff #177509)

This is required by IndexedMap, if I understand correctly.

23 ↗

(On Diff #177509)

This is required by IndexedMap, if I understand correctly.

77 ↗

(On Diff #177509)

There isn't an imminent addition version at this point.

This matcher isn't recursive: it's just looking at the entire node to see if it is a call to the inverse function. If an inverse is embedded as part of a deeper expression, it won't see it (e.g., there no hasDescendant in this matcher).

test/clang-tidy/Inputs/absl/time/time.h

1 ↗

(On Diff #177509)

I can do this, but it might take a bit to get the commit bit turned on.

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

12 ↗

(On Diff #177509)

I think there may be some confusion here (and that's entirely my fault. :) )

We should never get this expression as input to the check, since it doesn't compile (as you point out):

absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1));

Since absl::ToDoubleSeconds requires that its argument is an absl::Duration, but the expression absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1) results in a double, we can't get this as input.

There may be other expressions which could be input, but in practice they don't really happen. I've added a contrived example to the tests, but at some point the tests get too complex and confuse the fix matching infrastructure.

[JonasToth]

clang-tidy/abseil/DurationRewriter.cpp

77 ↗

(On Diff #177509)

Matchers are recursive. There will be a next match of the inner nodes below this node, just by AST traversal.

clang-tidy/abseil/DurationSubtractionCheck.cpp

38 ↗

(On Diff #177590)

Loc.isInvalid() too. You can pass in macros from the command line that result in invalid sourcelocations.

test/clang-tidy/Inputs/absl/time/time.h

1 ↗

(On Diff #177509)

Maybe, I (or someone else) have no problem commiting for you.

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

12 ↗

(On Diff #177509)

Your last sentence is the thing ;) Murphies Law will hit this check, too. In my opinion wrong transformations are very unfortunate and should be avoided if possible (in this case possible).
You can simply require that the expression of type double does not contain any duration subtraction calls.

This is even possible in the matcher-part of the check.

[hwright]

Rebase

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

12 ↗

(On Diff #177509)

I've written a test (which the testing infrastructure fails to handle well, so I haven't included it in the diff), and it produces these results:

   //
   //
-  x = absl::ToDoubleSeconds(d) - (absl::ToDoubleSeconds(d1) - 5);
+  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
   //
   //
-  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
+  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1 - absl::Seconds(5))));

Those results are correct. There is a cosmetic issue of round tripping through the double conversion in the absl::Seconds(absl::ToDoubleSeconds(...)) phrase, but untangling that is 1) difficult (because of order of operations issues) and thus; 2) probably the subject of a separate check.

This is still such a rare case (as in, we've not encountered it in Google's codebase), that I'm not really concerned. But if it's worth it to explicitly exclude it from the traversal matcher, I can do that.

[JonasToth]

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

12 ↗

(On Diff #177509)

Can you say what the direct issue is? I would bet its the overlapping?
A note in the documentation would be ok from my side. When the conflicting transformations are tried to be applied clang-tidy does not crash but print a nice diagnostic and continue its life?

[hwright]

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

12 ↗

(On Diff #177509)

Another example I've verified:

-  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
+  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5));

This a nested case, and while clang-tidy finds both of them, it only applies the outer most one (presumably the one it finds first in its AST traversal):

note: this fix will not be applied because it overlaps with another fix

The new code can then be checked again to fix the internal instance.

It's not possible to express this case in a test because testing infrastructure uses regular expressions, and the repeated strings in the test expectation cause it to get a bit confused.

Given all the of the above, I'm unsure what content would go in the documentation which is specific to this check.

[JonasToth]

Did you rebase the check onto the new master with your refactorings in?

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

12 ↗

(On Diff #177509)

Yes, that should be the outermost and the first instance it finds.
IMHO the documentation should say something along the lines of
Because it is possible the timing functions can be nested (as in one of your example) not all occurences in this single expression can be transformed in one run. Running clang-tidy multiple times will fix the nested instances, too.

The issue does not come from your check but the user will notice it through your check. And a short note for that won't hurt as it is not unreasonable someone might want to nest such expressions, even if uncommon.

[hwright]

Rebase and update documentation

[hwright]

I've updated the documentation, and the rebased to master.

[JonasToth]

LGTM with the doc-nit.

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

33 ↗

(On Diff #178039)

I think it the doc should be explicit that this is the case for nesting the subtraction expression the issue can occur. Otherwise it might be a bit confusing.

[hwright]

Thanks for reviewing, I'll go ahead and commit.

I've also removed the hash specialization, since we moved to llvm::IndexedMap instead of std::unordered_map inside of getInverseForScale.

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 "DurationSubtractionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "UpgradeDurationConversionsCheck.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<DurationSubtractionCheck>(
+        "abseil-duration-subtraction");
     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
   DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
+  DurationSubtractionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 

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

 #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"))))))

clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.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,
+enum class DurationScale : std::uint8_t {
+  Hours = 0,
   Minutes,
   Seconds,
   Milliseconds,
   Microseconds,
   Nanoseconds,
 };
-} // namespace abseil
-} // namespace tidy
-} // namespace clang
-
-namespace std {
-template <> struct hash<::clang::tidy::abseil::DurationScale> {
-  using argument_type = ::clang::tidy::abseil::DurationScale;
-  using underlying_type = std::underlying_type<argument_type>::type;
-  using result_type = std::hash<underlying_type>::result_type;
-
-  result_type operator()(const argument_type &arg) const {
-    std::hash<underlying_type> hasher;
-    return hasher(static_cast<underlying_type>(arg));
-  }
-};
-} // namespace std
 
-namespace clang {
-namespace tidy {
-namespace abseil {
 /// 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 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);
 
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name);
+
+/// Assuming `Node` has type `double` or `int` representing a time interval of
+/// `Scale`, return the expression to make it a suitable `Duration`.
+llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+    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",

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"
+#include "llvm/ADT/IndexedMap.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
+    return static_cast<unsigned>(Scale);
+  }
+};
+
 /// 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;
 }
 
+/// Given a `Scale` return the inverse functions for it.
+static const std::pair<llvm::StringRef, llvm::StringRef> &
+getInverseForScale(DurationScale Scale) {
+  static const llvm::IndexedMap<std::pair<llvm::StringRef, llvm::StringRef>,
+                                DurationScale2IndexFunctor>
+      InverseMap = []() {
+        // TODO: Revisit the immediately invoked lamba technique when
+        // IndexedMap gets an initializer list constructor.
+        llvm::IndexedMap<std::pair<llvm::StringRef, llvm::StringRef>,
+                         DurationScale2IndexFunctor>
+            InverseMap;
+        InverseMap.resize(6);
+        InverseMap[DurationScale::Hours] =
+            std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours");
+        InverseMap[DurationScale::Minutes] =
+            std::make_pair("::absl::ToDoubleMinutes", "::absl::ToInt64Minutes");
+        InverseMap[DurationScale::Seconds] =
+            std::make_pair("::absl::ToDoubleSeconds", "::absl::ToInt64Seconds");
+        InverseMap[DurationScale::Milliseconds] = std::make_pair(
+            "::absl::ToDoubleMilliseconds", "::absl::ToInt64Milliseconds");
+        InverseMap[DurationScale::Microseconds] = std::make_pair(
+            "::absl::ToDoubleMicroseconds", "::absl::ToInt64Microseconds");
+        InverseMap[DurationScale::Nanoseconds] = std::make_pair(
+            "::absl::ToDoubleNanoseconds", "::absl::ToInt64Nanoseconds");
+        return InverseMap;
+      }();
+
+  return InverseMap[Scale];
+}
+
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+rewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+                           DurationScale Scale, const Expr &Node) {
+  const std::pair<llvm::StringRef, llvm::StringRef> &InverseFunctions =
+      getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst<const Expr>(
+          "e",
+          match(callExpr(callee(functionDecl(hasAnyName(
+                             InverseFunctions.first, InverseFunctions.second))),
+                         hasArgument(0, expr().bind("e"))),
+                Node, *Result.Context))) {
+    return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }
+
+  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:
std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result,
   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();
 }
 
+llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::StringMap<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;
+}
+
+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 =
+          rewriteInverseDurationCall(Result, Scale, RootNode))
+    return *MaybeRewrite;
+
+  if (IsLiteralZero(Result, RootNode))
+    return std::string("absl::ZeroDuration()");
+
+  return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
+          simplifyDurationFactoryArg(Result, RootNode) + ")")
+      .str();
+}
+
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

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

+//===--- DurationSubtractionCheck.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_DURATIONSUBTRACTIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONSUBTRACTIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Checks for cases where subtraction should be performed in the
+/// `absl::Duration` domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-subtraction.html
+class DurationSubtractionCheck : public ClangTidyCheck {
+public:
+  DurationSubtractionCheck(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_DURATIONSUBTRACTIONCHECK_H

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

+//===--- DurationSubtractionCheck.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 "DurationSubtractionCheck.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 {
+
+void DurationSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      binaryOperator(
+          hasOperatorName("-"),
+          hasLHS(callExpr(callee(functionDecl(DurationConversionFunction())
+                                     .bind("function_decl")),
+                          hasArgument(0, expr().bind("lhs_arg")))))
+          .bind("binop"),
+      this);
+}
+
+void DurationSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("function_decl");
+
+  // Don't try to replace things inside of macro definitions.
+  if (Binop->getExprLoc().isMacroID() || Binop->getExprLoc().isInvalid())
+    return;
+
+  llvm::Optional<DurationScale> Scale = getScaleForInverse(FuncDecl->getName());
+  if (!Scale)
+    return;
+
+  llvm::Optional<std::string> RhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
+  if (!RhsReplacement)
+    return;
+
+  const Expr *LhsArg = Result.Nodes.getNodeAs<Expr>("lhs_arg");
+
+  diag(Binop->getBeginLoc(), "perform subtraction in the duration domain")
+      << FixItHint::CreateReplacement(
+             Binop->getSourceRange(),
+             (llvm::Twine("absl::") + FuncDecl->getName() + "(" +
+              tooling::fixit::getText(*LhsArg, *Result.Context) + " - " +
+              *RhsReplacement + ")")
+                 .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

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

- New :doc:`abseil-duration-factory-float
   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.
 
+- New :doc:`abseil-duration-subtraction
+  <clang-tidy/checks/abseil-duration-subtraction>` check.
+
+  Checks for cases where subtraction should be performed in the
+  ``absl::Duration`` domain.
+
 - 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-subtraction.rst

+.. title:: clang-tidy - abseil-duration-subtraction
+
+abseil-duration-subtraction
+===========================
+
+Checks for cases where subtraction should be performed in the
+``absl::Duration`` domain. When subtracting two values, and the first one is
+known to be a conversion from ``absl::Duration``, we can infer that the second
+should also be interpreted as an ``absl::Duration``, and make that inference
+explicit.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Subtraction in the double domain
+  double x;
+  absl::Duration d;
+  double result = absl::ToDoubleSeconds(d) - x;
+
+  // Suggestion - Subtraction in the absl::Duration domain instead
+  double result = absl::ToDoubleSeconds(d - absl::Seconds(x));
+
+
+  // Original - Subtraction of two Durations in the double domain
+  absl::Duration d1, d2;
+  double result = absl::ToDoubleSeconds(d1) - absl::ToDoubleSeconds(d2);
+
+  // Suggestion - Subtraction in the absl::Duration domain instead
+  double result = absl::ToDoubleSeconds(d1 - d2);
+
+Note: As with other ``clang-tidy`` checks, it is possible that multiple fixes
+may overlap (as in the case of nested expressions), so not all occurences can
+be transformed in one run. In particular, this may occur for nested subtraction
+expressions. Running ``clang-tidy`` multiple times will find and fix these
+overlaps.

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-duration-subtraction
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-string-find-startswith
    abseil-str-cat-append
    abseil-upgrade-duration-conversions
    android-cloexec-accept

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

+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1, d2;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // A nested occurance
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(5));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(5))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1)))
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}