[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:21
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
----------------
Are you using `argument_type`? Browser searching did only show one result.


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:23
+  unsigned operator()(DurationScale Scale) const {
+    return static_cast<unsigned>(Scale);
+  }
----------------
Why not `std::uint8_t` as its the underlying type for the `enum`?


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:73
+static llvm::Optional<std::string>
+maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+                                DurationScale Scale, const Expr &Node) {
----------------
`maybe` in the name is redundant, as its return type is `Optional`


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+      getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst<const Expr>(
+          "e",
----------------
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.


================
Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {
----------------
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.


================
Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // 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]
----------------
>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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245