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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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



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


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

https://reviews.llvm.org/D55245