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

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:21
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
----------------
JonasToth wrote:
> Are you using `argument_type`? Browser searching did only show one result.
This is required by `IndexedMap`, if I understand correctly.


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:23
+  unsigned operator()(DurationScale Scale) const {
+    return static_cast<unsigned>(Scale);
+  }
----------------
JonasToth wrote:
> Why not `std::uint8_t` as its the underlying type for the `enum`?
This is required by `IndexedMap`, if I understand correctly.


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


================
Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {
----------------
JonasToth wrote:
> 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.
I can do this, but it might take a bit to get the commit bit turned on.


================
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]
----------------
JonasToth wrote:
> 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.
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.


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

https://reviews.llvm.org/D55245