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

[Hyrum Wright via Phabricator via cfe-commits]

hwright marked 2 inline comments as done.
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:
> > > 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.
> 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?
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.


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

https://reviews.llvm.org/D55245