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

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.


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


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

https://reviews.llvm.org/D55245